[GitHub] danielcweeks commented on a change in pull request #22: ExpireSnapshots: do not commit when no snapshots are expired.

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

danielcweeks commented on a change in pull request #22: ExpireSnapshots: do not commit when no snapshots are expired.
URL: https://github.com/apache/incubator-iceberg/pull/22#discussion_r239538536
 
 

 ##########
 File path: core/src/main/java/com/netflix/iceberg/RemoveSnapshots.java
 ##########
 @@ -115,7 +115,10 @@ public void commit() {
         .onlyRetryOn(CommitFailedException.class)
         .run(item -> {
           TableMetadata updated = internalApply();
-          ops.commit(base, updated);
+          // only commit the updated metadata if at least one snapshot was removed
+          if (updated.snapshots().size() != base.snapshots().size()) {
 
 Review comment:
   I think those two checks are functionally equivalent for this check.  This is to effectively check that the number of snapshots has not changed.  I would err on the side of equality as the proposed change would conceptually mean that you could increase the number of snapshots, which would not be expected in this case.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services