JingsongLi commented on code in PR #10078:
URL: https://github.com/apache/paimon/pull/10078#discussion_r4068901235
##########
paimon-hive/paimon-hive-catalog/src/test/java/org/apache/paimon/hive/HiveCatalogTest.java:
##########
@@ -304,6 +304,76 @@ public void testAlterHiveTableParameters() {
}
}
+ @Test
+ public void testCreateExternalTableWithSchemelessLocation() throws
Exception {
+ // A `LOCATION '/path'` without a scheme must resolve against the
default filesystem rather
+ // than silently falling back to FileIO's local implementation, which
would write the
+ // schema files to the driver's local disk on a cluster.
+ String databaseName = "test_db";
+ String tableName = "external_table";
+ catalog.createDatabase(databaseName, false);
+ Identifier identifier = Identifier.create(databaseName, tableName);
+
+ String schemelessLocation = "/data/external/" + tableName;
Review Comment:
[P2] Allocate the schemeless test location under the test's temporary
directory
This fixture uses the default local filesystem, so `catalog.createTable`
actually tries to create `/data/external/external_table/schema`. On a normal
non-root checkout this fails before the assertions; running the two new tests
on JDK 8 produced `java.io.IOException: Mkdirs failed to create
file:/data/external/external_table/schema` here, while the rollback test
passed. The fixed global path also persists outside test cleanup. Please derive
a unique absolute path from the fixture's temporary directory and strip its URI
scheme; a configured non-local filesystem case should separately verify the
original wrong-filesystem regression.
##########
paimon-hive/paimon-hive-catalog/src/main/java/org/apache/paimon/hive/HiveCatalog.java:
##########
@@ -1270,14 +1284,48 @@ protected void createTableImpl(Identifier identifier,
Schema schema) {
location,
externalTable)));
} catch (Exception e) {
+ cleanupOnCreateTableFailure(identifier, location, externalTable);
+ throw new RuntimeException("Failed to create table " +
identifier.getFullName(), e);
+ }
+ }
+
+ /**
+ * Rolls back the table registration after {@code createHiveTable} failed,
so no zombie entry is
+ * left behind in the metastore. The metastore registration is the commit
point of {@link
+ * #createTableImpl(Identifier, Schema)}: the schema is written first, so
removing the table
+ * (and the schema files that have just been written for a managed table)
restores the pre-call
+ * state.
+ */
+ @VisibleForTesting
+ void cleanupOnCreateTableFailure(Identifier identifier, Path location,
boolean externalTable) {
+ try {
+ clients()
+ .execute(
+ client ->
+ client.dropTable(
+ identifier.getDatabaseName(),
+ identifier.getTableName(),
+ true,
+ false));
Review Comment:
[P1] Preserve the other creator's table when registration fails
`AbstractCatalog.createTable` checks table existence before entering
`createTableImpl`, and the metastore `createTable` call is outside
`runWithLock`. If another caller registers the same identifier in that
interval, our `createTable` throws `AlreadyExistsException`, but this catch now
unconditionally drops the other caller's table. This is reachable for external
creates, where an existing filesystem schema can be reused; with different
requested locations the winner can even own a completely different directory.
`deleteData=true` also makes a managed winner's data eligible for deletion. I
reproduced the loss of the winning HMS entry with a focused test against this
head; the same test passes with the previous catch behavior. Please remove this
rollback from the location fix, or require proven ownership of the registered
table and exclude `AlreadyExistsException` before deleting anything.
--
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]
