[GitHub] [iceberg] rymurr commented on a change in pull request #2588: Bump Nessie to 0.8.2 + related changes

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

rymurr commented on a change in pull request #2588:
URL: https://github.com/apache/iceberg/pull/2588#discussion_r670554615



##########
File path: nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
##########
@@ -174,26 +177,31 @@ public boolean dropTable(TableIdentifier identifier, boolean purge) {
         .build();
 
     // We try to drop the table. Simple retry after ref update.
-    boolean threw = true;
+    boolean ok = true;
     try {
       Tasks.foreach(contents)
           .retry(5)
           .stopRetryOn(NessieNotFoundException.class)
           .throwFailureWhenFinished()
           .onFailure((c, exception) -> refresh())
           .run(c -> {
-            client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(), reference.getHash(), c);
-            refresh(); // note: updated to reference.updateReference() with Nessie 0.6
+            Branch branch = client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),

Review comment:
       This in my honest opinion should be a different change. This is atomic imho

##########
File path: nessie/src/main/java/org/apache/iceberg/nessie/TableReference.java
##########
@@ -21,25 +21,28 @@
 
 import java.time.Instant;
 import java.util.List;
+import javax.annotation.Nonnull;
 import org.apache.iceberg.catalog.TableIdentifier;
 import org.apache.iceberg.relocated.com.google.common.base.Splitter;
 
 public class TableReference {
 
   private static final Splitter BRANCH_NAME_SPLITTER = Splitter.on("@");
+  @Nonnull

Review comment:
       what does this do?

##########
File path: nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java
##########
@@ -225,8 +233,13 @@ public void renameTable(TableIdentifier from, TableIdentifier toOriginal) {
           .throwFailureWhenFinished()
           .onFailure((c, exception) -> refresh())
           .run(c -> {
-            client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(), reference.getHash(), c);
-            refresh();
+            Branch branch = client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),

Review comment:
       imho this is a different change too but im less concerned about this.
   
   Why do we check if the branch is null...when can it be null




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