Core: If status check fails, commit should be unknown (#3717)

apache · Dec 12, 2021 · 4c48df2 · 4c48df2
1 parent d994549
commit 4c48df2
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 8 deletions.
diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java
@@ -306,8 +306,8 @@ protected CommitStatus checkCommitStatus(String newMetadataLocation, TableMetada
             LOG.info("Commit status check: Commit to {} of {} succeeded", tableName(), newMetadataLocation);
             status.set(CommitStatus.SUCCESS);
           } else {
-            LOG.info("Commit status check: Commit to {} of {} failed", tableName(), newMetadataLocation);
-            status.set(CommitStatus.FAILURE);
+            LOG.warn("Commit status check: Commit to {} of {} unknown, new metadata location is not current " +
+                    "or in history", tableName(), newMetadataLocation);
           }
         });
 

diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
@@ -77,10 +77,11 @@ public void testSuppressUnlockExceptions() throws TException, InterruptedExcepti
   }
 
   /**
-   * Pretends we throw an error while persisting that actually fails to commit serverside
+   * Pretends we throw an error while persisting, and not found with check state, commit state should be treated as
+   * unknown, because in reality the persisting may still succeed, just not yet by the time of checking.
    */
   @Test
-  public void testThriftExceptionFailureOnCommit() throws TException, InterruptedException {
+  public void testThriftExceptionUnknownStateIfNotInHistoryFailureOnCommit() throws TException, InterruptedException {
     Table table = catalog.loadTable(TABLE_IDENTIFIER);
     HiveTableOperations ops = (HiveTableOperations) ((HasTableOperations) table).operations();
 
@@ -100,14 +101,15 @@ public void testThriftExceptionFailureOnCommit() throws TException, InterruptedE
 
     failCommitAndThrowException(spyOps);
 
-    AssertHelpers.assertThrows("We should rethrow generic runtime errors if the " +
-        "commit actually doesn't succeed", RuntimeException.class, "Metastore operation failed",
-        () -> spyOps.commit(metadataV2, metadataV1));
+    AssertHelpers.assertThrows("We should assume commit state is unknown if the " +
+                    "new location is not found in history in commit state check", CommitStateUnknownException.class,
+            "Datacenter on fire", () -> spyOps.commit(metadataV2, metadataV1));
 
     ops.refresh();
     Assert.assertEquals("Current metadata should not have changed", metadataV2, ops.current());
     Assert.assertTrue("Current metadata should still exist", metadataFileExists(metadataV2));
-    Assert.assertEquals("No new metadata files should exist", 2, metadataFileCount(ops.current()));
+    Assert.assertEquals("New metadata files should still exist, new location not in history but" +
+                    " the commit may still succeed", 3, metadataFileCount(ops.current()));
   }
 
   /**