[GitHub] [iceberg] rdblue commented on a change in pull request #1384: Remove a wrong check for commit operation

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #1384:
URL: https://github.com/apache/iceberg/pull/1384#discussion_r477727448



##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -156,10 +156,7 @@ public void commit() {
         .onlyRetryOn(CommitFailedException.class)
         .run(item -> {
           TableMetadata updated = internalApply();
-          // only commit the updated metadata if at least one snapshot was removed
-          if (updated.snapshots().size() != base.snapshots().size()) {
-            ops.commit(base, updated);
-          }

Review comment:
       How about also updating `internalApply` so that it does this check?
   
   This is intended to only modify the table and run a commit if the table changes. But you're right that when combined with a transaction, this will cause a failure because commit is not called at all.
   
   But we still want to avoid unnecessary commits. To do that, `internalApply` should do this check on the new `TableMetadata` it produces. If the new metadata has the same set of manifests as the original, then it should return the original. That will cause the commit to be skipped in most cases because the actual commit logic inside of `ops.commit` checks whether the metadata is the same as the base metadata and returns if it is.




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

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