[GitHub] [iceberg] rdblue commented on a diff in pull request #6634: Core: Fix for deleting files when commiting transactions with multiple branches

["rdblue (via GitHub)" <gi...@apache.org>]

rdblue commented on code in PR #6634:
URL: https://github.com/apache/iceberg/pull/6634#discussion_r1084371360


##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -433,17 +425,18 @@ private void commitSimpleTransaction() {
     // the commit succeeded
 
     try {
-      if (currentSnapshotId.get() != -1) {
-        intermediateSnapshotIds.add(currentSnapshotId.get());
+      // clean up the data files that were deleted by each operation. first, get the list of
+      // committed manifests to ensure that no committed manifest is deleted.
+      // A manifest could be deleted in one successful operation commit, but reused in another
+      // successful commit of that operation if the whole transaction is retried.
+      Set<Long> currentSnapshots = Sets.newHashSet();

Review Comment:
   This isn't really "current". I think it's the snapshots that were added in the transaction. Minor, but I'd probably call it `newSnapshots`.



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org