stevenzwu commented on code in PR #18074:
URL: https://github.com/apache/iceberg/pull/18074#discussion_r4477425716
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -108,7 +109,9 @@ public void accept(String file) {
private final String commitUUID = UUID.randomUUID().toString();
private final AtomicInteger manifestCount = new AtomicInteger(0);
private final AtomicInteger attempt = new AtomicInteger(0);
- private final List<String> manifestLists = Lists.newArrayList();
+
+ private final Map<String, Set<String>> manifestsByManifestList =
Maps.newHashMap();
Review Comment:
Nit: the blank lines on either side of this new field break the grouping of
the surrounding fields. I would drop both.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -610,13 +615,36 @@ private void notifyListeners() {
}
protected void cleanAll() {
- for (String manifestList : manifestLists) {
+ for (String manifestList : manifestsByManifestList.keySet()) {
deleteFile(manifestList);
}
- manifestLists.clear();
+ manifestsByManifestList.clear();
cleanUncommitted(EMPTY_SET);
}
+ /**
+ * Returns the manifest paths of the committed snapshot without reading its
manifest list when
+ * this producer wrote that manifest list.
+ */
+ private Set<String> committedManifestPaths(Snapshot saved) {
+ Set<String> paths = writtenManifestPaths(saved.manifestListLocation());
+ if (paths != null) {
+ return paths;
+ }
+
+ return saved.allManifests(ops.io()).stream()
Review Comment:
```suggestion
// this producer did not write the manifest list (e.g. a fast-forward
cherry-pick), so read it
return saved.allManifests(ops.io()).stream()
```
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -455,17 +455,33 @@ private void applyUpdates(TableOperations underlyingOps) {
}
// returns the manifest lists and manifests referenced by the given
committed snapshots
- private static Set<String> committedFiles(FileIO io, Set<Snapshot>
snapshots) {
+ private Set<String> committedFiles(Set<Snapshot> snapshots) {
Set<String> committedFiles = Sets.newHashSet();
for (Snapshot snap : snapshots) {
committedFiles.add(snap.manifestListLocation());
- snap.allManifests(io).forEach(manifest ->
committedFiles.add(manifest.path()));
+ committedFiles.addAll(committedManifestPaths(snap));
}
return committedFiles;
}
+ // uses the manifest paths held by the update that wrote the snapshot's
manifest list, if any
Review Comment:
Javadoc describes the contract of this method.
```suggestion
/** Returns this snapshot's manifest paths. */
```
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -455,17 +455,33 @@ private void applyUpdates(TableOperations underlyingOps) {
}
// returns the manifest lists and manifests referenced by the given
committed snapshots
- private static Set<String> committedFiles(FileIO io, Set<Snapshot>
snapshots) {
+ private Set<String> committedFiles(Set<Snapshot> snapshots) {
Set<String> committedFiles = Sets.newHashSet();
for (Snapshot snap : snapshots) {
committedFiles.add(snap.manifestListLocation());
- snap.allManifests(io).forEach(manifest ->
committedFiles.add(manifest.path()));
+ committedFiles.addAll(committedManifestPaths(snap));
}
return committedFiles;
}
+ // uses the manifest paths held by the update that wrote the snapshot's
manifest list, if any
+ private Set<String> committedManifestPaths(Snapshot snapshot) {
+ for (PendingUpdate update : updates) {
+ if (update instanceof SnapshotProducer<?> producer) {
+ Set<String> paths =
producer.writtenManifestPaths(snapshot.manifestListLocation());
+ if (paths != null) {
+ return paths;
+ }
+ }
+ }
+
+ return snapshot.allManifests(ops.io()).stream()
Review Comment:
This fallback handles snapshots that none of this transaction updates wrote.
I do not see a test that reaches this line, so the read path for that case is
unverified. A test that commits a snapshot outside the transaction updates and
checks that the manifest list is read would cover it.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -610,13 +615,36 @@ private void notifyListeners() {
}
protected void cleanAll() {
- for (String manifestList : manifestLists) {
+ for (String manifestList : manifestsByManifestList.keySet()) {
deleteFile(manifestList);
}
- manifestLists.clear();
+ manifestsByManifestList.clear();
cleanUncommitted(EMPTY_SET);
}
+ /**
+ * Returns the manifest paths of the committed snapshot without reading its
manifest list when
Review Comment:
Javadoc documents the method contract. this describes the implementation
behavior, which can be better served by the two inline comments suggested
below. I would drop this Javadoc.
##########
core/src/test/java/org/apache/iceberg/TestSnapshotProducer.java:
##########
@@ -403,4 +408,74 @@ public void monotonicityIsScopedToTargetBranch() {
.isEqualTo(mainNewTs)
.isLessThan(branchTs);
}
+
+ @TestTemplate
+ public void commitDoesNotReadCommittedManifestList() {
Review Comment:
Could we add the fallback case here? A fast-forward cherry-pick returns the
existing snapshot without writing a manifest list. This producer then has no
entry in `manifestsByManifestList`, so `committedManifestPaths` reads the list
from storage. The producer creates no manifests, but that read is still the
path that decides what cleanup keeps.
Suggested test: cherry-pick a snapshot onto a branch that is a fast-forward,
spy `newInputFile` as in this test, and assert it is called for the committed
manifest list. Also assert that the manifests the cherry-picked snapshot
references still exist after commit.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -610,13 +615,36 @@ private void notifyListeners() {
}
protected void cleanAll() {
- for (String manifestList : manifestLists) {
+ for (String manifestList : manifestsByManifestList.keySet()) {
deleteFile(manifestList);
}
- manifestLists.clear();
+ manifestsByManifestList.clear();
cleanUncommitted(EMPTY_SET);
}
+ /**
+ * Returns the manifest paths of the committed snapshot without reading its
manifest list when
+ * this producer wrote that manifest list.
+ */
+ private Set<String> committedManifestPaths(Snapshot saved) {
+ Set<String> paths = writtenManifestPaths(saved.manifestListLocation());
+ if (paths != null) {
Review Comment:
maybe an inline comment like this.
```
// paths were recorded when this producer wrote the manifest list. Return
manifests from memory.
```
--
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]
