pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956347020
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -596,6 +600,63 @@ private void concurrentCommitAndThrowException(
.persistTable(any(), anyBoolean(), any());
}
+ /**
+ * Pins the table-specific doRefresh wiring for a never-persisted table: a
CREATE TABLE commit
+ * that fails with a non-specific exception must resolve the commit status
cleanly instead of
+ * NPE-ing in checkCurrentMetadataLocation (#17462). refresh() yields null
current metadata and
+ * the status check supplier resolves to false (a new metadata location
cannot be current for a
+ * table that does not exist). The relaxed check then maps that to UNKNOWN.
+ */
+ @Test
+ public void
testThriftExceptionUnknownStateOnCreateCommitWhenTableNeverPersisted()
+ throws TException, InterruptedException, IOException {
+ TableIdentifier createIdentifier = TableIdentifier.of(DB_NAME,
"create_commit_failed_table");
+ HiveTableOperations ops = (HiveTableOperations)
catalog.newTableOps(createIdentifier);
+ HiveTableOperations spyOps = spy(ops);
+
+ failCommitAndThrowException(spyOps);
+
+ Path createLocation = getTableLocationPath("create_commit_failed_table");
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ createLocation.toString(),
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"));
+
+ try {
+ assertThatThrownBy(() -> spyOps.commit(null, metadata))
+ .isInstanceOf(CommitStateUnknownException.class)
+ .hasMessageStartingWith("Datacenter on fire");
+
+ assertThat(catalog.tableExists(createIdentifier))
+ .as("The table should not have been created")
+ .isFalse();
+
+ // pins the table-specific doRefresh wiring: a missing table is not an
error when no
+ // metadata location is known, so refreshing a never-persisted table
must yield null metadata
+ assertThat(ops.refresh())
+ .as("Refreshing a never-persisted table should yield null metadata")
+ .isNull();
+
+ // and the commit status check supplier must resolve to false for the
null metadata instead
+ // of throwing an NPE
+ assertThat(
+ checkCurrentMetadataLocation(
+ ops, createLocation + "/metadata/00000-uuid.metadata.json"))
+ .as("A new metadata location cannot be current for a never-persisted
table")
Review Comment:
Does this add value?
Is there any change in the actual commit results?
Shall we just create an TestHiveTableOperations if we want to ensure that
the HiveTableOperations does the same as the BaseMetastoreTableOperations?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
