amogh-jahagirdar commented on code in PR #18074:
URL: https://github.com/apache/iceberg/pull/18074#discussion_r4437409758
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -109,6 +112,10 @@ public void accept(String file) {
private final AtomicInteger manifestCount = new AtomicInteger(0);
private final AtomicInteger attempt = new AtomicInteger(0);
private final List<String> manifestLists = Lists.newArrayList();
+
+ /** Manifest paths written to each manifest list, keyed by manifest list
location. */
+ private final Map<String, List<String>> manifestPathsByManifestList =
Maps.newHashMap();
Review Comment:
I think we can simplify the state here a bit. `manifestLists` and the new
map are tracking the same thing, so imo we can just keep the map, record the
entry where `manifestLists.add` is today (paths are known before the write
since `addMetadata` keeps the path), and store a `Set` since both consumers
want one anyways. Something like:
```java
private final Map<String, Set<String>> manifestPathsByManifestList =
Maps.newLinkedHashMap();
...
try (writer) {
// keep track of the manifest lists created and the manifests they
reference
manifestPathsByManifestList.put(
manifestList.location(),
manifests.stream().map(ManifestFile::path).collect(Collectors.toSet()));
...
writer.addAll(Arrays.asList(manifestFiles));
}
```
then cleanup just iterates over `keySet()`:
```java
for (String manifestList : manifestPathsByManifestList.keySet()) {
if (!saved.manifestListLocation().equals(manifestList)) {
deleteFile(manifestList);
}
}
```
```java
protected void cleanAll() {
manifestPathsByManifestList.keySet().forEach(this::deleteFile);
manifestPathsByManifestList.clear();
cleanUncommitted(EMPTY_SET);
}
```
and `committedManifestPaths` no longer needs the copy:
```java
private Set<String> committedManifestPaths(Snapshot saved) {
Set<String> paths = writtenManifestPaths(saved.manifestListLocation());
if (paths != null) {
return paths;
}
return saved.allManifests(ops.io()).stream()
.map(ManifestFile::path)
.collect(Collectors.toSet());
}
```
Partial writes still get cleaned up since the entry exists before we write
anything, and a failed attempt's paths never get read since that manifest list
can't be committed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
