[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5888: Core: Rollback compaction on conflicts

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

singhpk234 commented on code in PR #5888:
URL: https://github.com/apache/iceberg/pull/5888#discussion_r1120865320


##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
       }
     }
   }
+
+  private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+    Long newParentSnapshotId = parentSnapshotId;
+    if (shouldRollbackReplaceOnConflict()) {
+      // add a set snapshot op on top of base to roll back to parent snapshot
+      boolean isCommitSuccessfullyApplied = false;
+      // Update parentSnapshot to it's grandParent

Review Comment:
   yup



##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -215,7 +219,18 @@ public Snapshot apply() {
     long sequenceNumber = base.nextSequenceNumber();
     Long parentSnapshotId = parentSnapshot == null ? null : parentSnapshot.snapshotId();
 
-    validate(base, parentSnapshot);
+    try {
+      validate(base, parentSnapshot);
+    } catch (ValidationException ve) {
+      parentSnapshotId = snapshotIdToRollbackToOnConflict(parentSnapshotId);

Review Comment:
   sounds good, made the changes 



##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -615,4 +630,49 @@ private static void updateTotal(
       }
     }
   }
+
+  private Long snapshotIdToRollbackToOnConflict(Long parentSnapshotId) {
+    Long newParentSnapshotId = parentSnapshotId;
+    if (shouldRollbackReplaceOnConflict()) {
+      // add a set snapshot op on top of base to roll back to parent snapshot
+      boolean isCommitSuccessfullyApplied = false;
+      // Update parentSnapshot to it's grandParent
+      // provided parentSnapshot is of type replace and never rollback beyond startingSnapshotId
+      while (newParentSnapshotId != null
+          && DataOperations.REPLACE.equals(base.snapshot(newParentSnapshotId).operation())
+          && !newParentSnapshotId.equals(startingSnapshotId())) {
+        // create a tempTableOperation to pass base with rollback to validate of update
+        TableOperations tempTableOps = ops.temp(base);
+        newParentSnapshotId = base.snapshot(newParentSnapshotId).parentId();
+        if (newParentSnapshotId == null) {
+          return null;
+        }
+        Snapshot parentSnapshot = base.snapshot(newParentSnapshotId);
+
+        SetSnapshotOperation setSnapshotOp = new SetSnapshotOperation(tempTableOps);
+        setSnapshotOp.rollbackTo(newParentSnapshotId).commit();

Review Comment:
   As per my understanding, since this is based on `TableOperations tempTableOps = ops.temp(base);` so it should not commit, we use this for validation to create a new temp metadata which will act as an uncommitted table metadata similar to what's done in transaction.



-- 
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