Copilot commented on code in PR #6872:
URL: https://github.com/apache/hive/pull/6872#discussion_r4274058554
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -171,13 +171,27 @@ public class HiveIcebergMetaHook extends
BaseHiveIcebergMetaHook {
private AlterTableType currentAlterTableOp;
private HiveLock commitLock;
private List<SQLDefaultConstraint> sqlDefaultConstraints;
+ private boolean commitPhaseStarted;
Review Comment:
`commitPhaseStarted` is set to `true` in `commitCreateTable` but is never
reset. If the same `HiveIcebergMetaHook` instance is reused across multiple
CREATE operations, a later rollback could incorrectly satisfy
`commitPhaseStarted` and reuse stale `tableProperties`, potentially dropping
the wrong table. Consider resetting the hook state at the start/end of a CREATE
lifecycle (e.g., set `commitPhaseStarted=false` and clear `tableProperties` in
the appropriate pre/commit/rollback paths) so rollback decisions cannot be
influenced by a prior operation.
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -171,13 +171,27 @@ public class HiveIcebergMetaHook extends
BaseHiveIcebergMetaHook {
private AlterTableType currentAlterTableOp;
private HiveLock commitLock;
private List<SQLDefaultConstraint> sqlDefaultConstraints;
+ private boolean commitPhaseStarted;
public HiveIcebergMetaHook(Configuration conf) {
super(conf);
}
+ @Override
+ public void rollbackCreateTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
+ if (commitPhaseStarted && tableProperties != null) {
+ try {
+ Catalogs.dropTable(conf, tableProperties);
+ } catch (Exception e) {
+ LOG.warn("Failed to clean up table during rollback of CREATE TABLE for
{}.{}",
+ hmsTable.getDbName(), hmsTable.getTableName(), e);
+ }
+ }
+ }
+
@Override
public void commitCreateTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
Review Comment:
`commitPhaseStarted` is set to `true` in `commitCreateTable` but is never
reset. If the same `HiveIcebergMetaHook` instance is reused across multiple
CREATE operations, a later rollback could incorrectly satisfy
`commitPhaseStarted` and reuse stale `tableProperties`, potentially dropping
the wrong table. Consider resetting the hook state at the start/end of a CREATE
lifecycle (e.g., set `commitPhaseStarted=false` and clear `tableProperties` in
the appropriate pre/commit/rollback paths) so rollback decisions cannot be
influenced by a prior operation.
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -171,13 +171,27 @@ public class HiveIcebergMetaHook extends
BaseHiveIcebergMetaHook {
private AlterTableType currentAlterTableOp;
private HiveLock commitLock;
private List<SQLDefaultConstraint> sqlDefaultConstraints;
+ private boolean commitPhaseStarted;
public HiveIcebergMetaHook(Configuration conf) {
super(conf);
}
+ @Override
+ public void rollbackCreateTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
+ if (commitPhaseStarted && tableProperties != null) {
+ try {
+ Catalogs.dropTable(conf, tableProperties);
+ } catch (Exception e) {
+ LOG.warn("Failed to clean up table during rollback of CREATE TABLE for
{}.{}",
+ hmsTable.getDbName(), hmsTable.getTableName(), e);
+ }
+ }
+ }
+
@Override
public void commitCreateTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
+ commitPhaseStarted = true;
if (HiveTableUtil.isIcebergView(hmsTable)) {
tableProperties = IcebergTableProperties.getTableProperties(hmsTable,
conf);
Review Comment:
Rollback cleanup is skipped when `tableProperties` is null, even if the
commit phase has started. If the failure happens before `tableProperties` is
populated for this operation, the HMS orphan problem may persist. A more robust
approach is to derive the identifier/properties needed to drop from `hmsTable`
(db/table name) when `tableProperties` is not available, or ensure
`tableProperties` is always initialized before entering commit work.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -720,6 +720,51 @@ public void testCreateTableError() {
}
}
+ @Test
+ public void testCreateTableRollbackCleansUpHMSEntry() {
+ Assume.assumeTrue("Rollback cleanup is only relevant for HiveCatalog",
+ testTableType == TestTableType.HIVE_CATALOG);
+
+ // Creating with format-version=2 should fail, and the HMS entry should be
cleaned up.
+ Assertions.assertThatThrownBy(
+ () -> shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='2')"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("timestamp_ns is not supported until v3");
Review Comment:
This assertion is fairly brittle (exact exception type and a very specific
message fragment). If upstream/driver layers wrap exceptions differently, the
test may fail for non-behavioral reasons. Consider asserting on the root cause
(or a broader exception contract) and/or checking a more stable portion of the
error (e.g., format-version incompatibility) rather than a specific wording.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -720,6 +720,51 @@ public void testCreateTableError() {
}
}
+ @Test
+ public void testCreateTableRollbackCleansUpHMSEntry() {
+ Assume.assumeTrue("Rollback cleanup is only relevant for HiveCatalog",
+ testTableType == TestTableType.HIVE_CATALOG);
+
+ // Creating with format-version=2 should fail, and the HMS entry should be
cleaned up.
+ Assertions.assertThatThrownBy(
+ () -> shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='2')"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("timestamp_ns is not supported until v3");
+
+ // Without the rollback fix, this would fail with AlreadyExistsException
+ // because the HMS entry from the failed create would still exist.
+ shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='3')");
+
+ shell.executeStatement("DROP TABLE rollback_test");
Review Comment:
The tests use fixed table names and perform cleanup only at the end. If an
assertion fails mid-test (or tests run concurrently), these names can collide
and/or leave residue that affects subsequent tests. Prefer unique table names
(e.g., with a random/UUID suffix) and ensure cleanup via `try/finally`
(drop-if-exists) so the metastore is left clean even on failure.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -720,6 +720,51 @@ public void testCreateTableError() {
}
}
+ @Test
+ public void testCreateTableRollbackCleansUpHMSEntry() {
+ Assume.assumeTrue("Rollback cleanup is only relevant for HiveCatalog",
+ testTableType == TestTableType.HIVE_CATALOG);
+
+ // Creating with format-version=2 should fail, and the HMS entry should be
cleaned up.
+ Assertions.assertThatThrownBy(
+ () -> shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='2')"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("timestamp_ns is not supported until v3");
+
+ // Without the rollback fix, this would fail with AlreadyExistsException
+ // because the HMS entry from the failed create would still exist.
+ shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='3')");
+
+ shell.executeStatement("DROP TABLE rollback_test");
+ }
+
+ @Test
+ public void testCreateTableRollbackDoesNotDropExistingTable() {
+ Assume.assumeTrue("Rollback cleanup is only relevant for HiveCatalog",
+ testTableType == TestTableType.HIVE_CATALOG);
+
+ shell.executeStatement(
+ "CREATE TABLE existing_tbl (id INT) STORED BY ICEBERG STORED AS
PARQUET");
Review Comment:
The tests use fixed table names and perform cleanup only at the end. If an
assertion fails mid-test (or tests run concurrently), these names can collide
and/or leave residue that affects subsequent tests. Prefer unique table names
(e.g., with a random/UUID suffix) and ensure cleanup via `try/finally`
(drop-if-exists) so the metastore is left clean even on failure.
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java:
##########
@@ -720,6 +720,51 @@ public void testCreateTableError() {
}
}
+ @Test
+ public void testCreateTableRollbackCleansUpHMSEntry() {
+ Assume.assumeTrue("Rollback cleanup is only relevant for HiveCatalog",
+ testTableType == TestTableType.HIVE_CATALOG);
+
+ // Creating with format-version=2 should fail, and the HMS entry should be
cleaned up.
+ Assertions.assertThatThrownBy(
+ () -> shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='2')"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("timestamp_ns is not supported until v3");
+
+ // Without the rollback fix, this would fail with AlreadyExistsException
+ // because the HMS entry from the failed create would still exist.
+ shell.executeStatement(
+ "CREATE TABLE rollback_test (id INT, ts TIMESTAMP(9)) " +
+ "STORED BY ICEBERG STORED AS PARQUET " +
+ "TBLPROPERTIES ('format-version'='3')");
+
+ shell.executeStatement("DROP TABLE rollback_test");
+ }
+
+ @Test
+ public void testCreateTableRollbackDoesNotDropExistingTable() {
+ Assume.assumeTrue("Rollback cleanup is only relevant for HiveCatalog",
+ testTableType == TestTableType.HIVE_CATALOG);
+
+ shell.executeStatement(
+ "CREATE TABLE existing_tbl (id INT) STORED BY ICEBERG STORED AS
PARQUET");
+
+ // Attempting to create a table with the same name should fail with
AlreadyExistsException.
+ // The rollback must NOT drop the existing table.
+ Assertions.assertThatThrownBy(
+ () -> shell.executeStatement(
+ "CREATE TABLE existing_tbl (id INT) STORED BY ICEBERG STORED AS
PARQUET"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("already exists");
+
+ // The original table should still be intact.
+ shell.executeStatement("DESCRIBE existing_tbl");
+ shell.executeStatement("DROP TABLE existing_tbl");
Review Comment:
The tests use fixed table names and perform cleanup only at the end. If an
assertion fails mid-test (or tests run concurrently), these names can collide
and/or leave residue that affects subsequent tests. Prefer unique table names
(e.g., with a random/UUID suffix) and ensure cleanup via `try/finally`
(drop-if-exists) so the metastore is left clean even on failure.
--
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]
