Repository navigation
[hive] Fix schemeless LOCATION for external tables and roll back failed creates - #10078
zhang-arvin wants to merge 5 commits into
Conversation
|
The Evidence — the same jobs fail on a clean
This PR only touches This comment was generated by an AI agent (Hermes Agent). |
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed 797b4b7772cc99931368d34eb7d4a561b248f2ba against the linked issue and the Spark → HiveCatalog → filesystem/metastore create path. Resolving a schemeless external LOCATION against the configured default filesystem has clear end-to-end value, so this PR should remain open.
The unconditional metastore rollback introduces a concurrent-create regression, and the new LOCATION test depends on writing to /data; details are inline. The reported Spark loadTable failure occurs after catalog.createTable returns, so this new HiveCatalog catch block does not handle that original failure either. Keeping this fix focused on location resolution would avoid that extra failure mode.
Validation on JDK 8: the two added tests produced one pass and one error (Mkdirs failed to create file:/data/external/external_table/schema). A focused reviewer test injected a competing HMS create between the existence check and registration: this head deleted the winning table; restoring only the previous create-failure cleanup behavior made the same test pass. I have not run the original Spark + HDFS reproduction end to end.
| client.dropTable( | ||
| identifier.getDatabaseName(), | ||
| identifier.getTableName(), | ||
| true, | ||
| false)); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Revalidated on 8fa791d133: excluding AlreadyExistsException fixes that specific response, but LOCATION equality still does not prove ownership for a generic RPC failure. I reproduced two real external HiveCatalog.createTable calls using the identical explicit location: the first stages schema, the competing call reuses that schema, registers the table and commits a row; the losing create RPC then throws TException before registering anything. cleanupOnCreateTableFailure sees the winner at the same location and deletes its HMS entry. The same test passes when only the generic catch is restored to its previous cleanup behavior. This is the remaining ownership problem from this thread, rather than a duplicate finding about the now-excluded AlreadyExists response. Please keep an ambiguous metastore outcome non-destructive, or establish a per-create identity and a protected conditional cleanup protocol; matching paths cannot distinguish these callers.
| catalog.createDatabase(databaseName, false); | ||
| Identifier identifier = Identifier.create(databaseName, tableName); | ||
|
|
||
| String schemelessLocation = "/data/external/" + tableName; |
There was a problem hiding this 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.
…ed creates Creating an external table with a schemeless LOCATION (for example LOCATION '/data/external/t') writes the schema to the driver's local filesystem instead of the configured default filesystem, leaves a zombie entry in the metastore when the create fails, and makes the subsequent loadTable call in SparkCatalog#createTable miss with NoSuchTableException. Two problems: 1. HiveCatalog#initialTableLocation builds the location straight from the LOCATION property. FileIO#get treats a scheme-less path as local, so schema-0 lands on local disk while the metastore points the table at the path that was requested. Resolve the scheme against FileSystem.getDefaultUri so the default filesystem is used, matching the behaviour of the catalog warehouse. Applies to both createTableImpl and createObjectTable. 2. The failure handler only deleted the directory for a managed table and never removed the metastore entry, so a failed create left the table registered in HMS with no readable schema. Roll the entry back via dropTable(..., true, false) and keep the managed-table directory cleanup.
797b4b7 to
97a0608
Compare
The test hard coded /data/external/external_table, which on the CI runner resolved to file:/data/external/external_table; creating the schema directory failed (java.io.IOException: Mkdirs failed to create file:/data/external/external_table/schema at HiveCatalog.createTableImpl(HiveCatalog.java:1270)) and Java / Core and integrations JDK 8 and JDK 11 failed with HiveCatalogTest.testCreateExternalTableWithSchemelessLocation:338 » Runtime Fa... - Failed to create table test_db.external_table. Take the location from @tempdir: still scheme free, but writable.
Wrapping the @tempdir parameter onto its own line does not match google-java-format, so spotless:check failed on paimon-hive-catalog before any test ran. That gate is a prerequisite of every build, which is why Java / Flink 1 / Common, Flink 1 / Connectors and CDC, Flink 2 / Common and connectors, E2E / Flink 2, E2E / Flink 1 and Spark interoperability, Spark 3 (2.12, 2.13), Spark 4 and Iceberg GA all failed with the same 'Maven spotless:check ... format violations: src/test/java/org/apache/paimon/hive/HiveCatalogTest.java'. Keep the parameter on the first line and wrap throws instead, matching the expected output.
|
Revalidated on I reproduced this through real Please remove this cleanup from the location fix, or establish ownership of the registered table before deleting it, including excluding The schemeless-location fix has useful end-to-end value. All 58 |
The rollback added with the schemeless-location fix dropped the metastore entry unconditionally. If another caller registered the same identifier while this call was writing its schema (reachable for external creates that reuse an existing filesystem schema), the AlreadyExistsException path dropped the winner's table, and for a managed winner also deleted its data files. Cleanup now only runs when this call can prove it owns the registered entry: the table stored in the metastore must point at the location this call resolved. AlreadyExistsException skips the drop entirely, and only the schema files written by the losing call are removed. Adds a regression test for the competing-create case.
|
Addressed the P1 ownership finding on head
The schemeless-location fix (P1 does not touch it) is unchanged. Compile, checkstyle and spotless pass locally; the HMS-backed tests need JDK 8 in this environment ( This comment was generated by an AI agent (Hermes Agent). |
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED for resolving schemeless external locations. The temporary-directory fixture and AlreadyExistsException case are repaired, but the ownership blocker remains on generic RPC failures (reproduction added to the existing thread).
Normal Maven checks/JDK 8: all 59 existing embedded-HMS catalog tests passed. Additional probes confirm the remaining ownership failure, the managed-create retry failure and the relative-location regression below. Restoring only the previous generic create-failure cleanup makes both ownership/retry probes pass (2/2). The JDK 11 CI failed resolving a ScalaTest plugin dependency; no actual Spark/HDFS deployment was run locally.
| LOG.info( | ||
| "Skipping cleanup of [{}]: the registered table is not owned by this call.", | ||
| identifier.getFullName()); | ||
| return; |
There was a problem hiding this comment.
[P2] Preserve recovery of a managed create that never registered
When schema-0 has been written but the HMS create RPC fails before registering the table, ownsRegisteredTable returns false and this early return also skips cleanup of the schema written by this attempt. After restoring the RPC, a real HiveCatalog.createTable retry fails with "Schema in filesystem exists, creation is not allowed", although HMS has no entry. A focused embedded-HMS fault-injection test reproduces this; restoring the previous managed-create cleanup makes it pass. Keep metastore ownership decisions separate from recovery of this attempt's schema, with a concurrency-safe ownership check for any filesystem deletion, or keep this PR focused on location resolution and handle create recovery separately.
| if (location.toUri().getScheme() != null) { | ||
| return location; | ||
| } | ||
| return new Path(FileSystem.getDefaultUri(hiveConf).toString(), location); |
There was a problem hiding this comment.
[P2] Qualify relative locations against the working directory
The catalog path option accepts relative locations. With fs.defaultFS=file:///, resolving target/external-t as a child of the default filesystem URI makes it file:/target/external-t, whereas the previous LocalFileIO path refers to target/external-t under the process working directory. Existing data there is missed, or creation fails trying to write under /. A focused resolver regression observes /target/review-relative-external instead of the catalog test's working-directory path. Use filesystem qualification with its working directory for relative paths while preserving scheme/authority inheritance for absolute schemeless paths, and cover a relative-location create case.
2 participants
What is the purpose of the change
Fixes #9990.
CREATE TABLE ... LOCATION '/data/external/t'(a LOCATION without a URI scheme) on aHiveCatalogwrites the schema to the driver's local filesystem instead of the configured default filesystem, leaves a zombie entry in the metastore when the create fails, and then fails withNoSuchTableExceptionout ofSparkCatalog#createTable.Brief change log
1.
HiveCatalog#initialTableLocationbuilt the location without resolving the scheme.FileIO#get(FileIO.java:527) returns aLocalFileIOwhenever the path has no scheme, so for a schemelessLOCATIONthe catalog wroteschema-0to the driver's local disk while the metastore registered the table against the path the user asked for. The table is then unreadable from any other node, and the log lines in the report match this exactly (schema-0on the local FS, empty directory on HDFS).resolveLocationSchemenow resolves a scheme-less location againstFileSystem.getDefaultUri(hiveConf), so it lands on the default filesystem like the catalog warehouse does. Already-schemed locations are returned untouched. Applied to bothcreateTableImplandcreateObjectTable.2. A failed create left the metastore entry behind.
The
catchblock aroundcreateHiveTabledeleted the directory only for a managed table and never removed the metastore entry. The schema write happens before the registration, so removing the entry restores the pre-call state.cleanupOnCreateTableFailurenow drops the table viadropTable(db, table, true, false)and keeps the managed-table directory cleanup.Note on the Spark-side symptom
The
NoSuchTableException/zombie-table behaviour the report observes was introduced by #9549, which madeSparkCatalog#createTablecallloadTable(ident)right aftercatalog.createTable(...). That change is reasonable when the table is genuinely readable, so the fix here is at the catalog layer where the broken location and the missing rollback actually live — theloadTablethen succeeds instead of miss. Happy to restructure it if maintainers prefer the resolution to live inSparkCataloginstead.Tests
testCreateExternalTableWithSchemelessLocation— creates an external table withpath=/data/external/...and asserts the location the catalog works with carries an explicit scheme and preserves the requested path. This asserts the regressed behaviour directly and fails without the fix.testCreateTableDoesNotLeaveZombieEntryWhenMetastoreFails— asserts the rollback path removes the metastore entry.Verification status
mvn -pl paimon-hive/paimon-hive-catalog -am -Pfast-build -DskipTests compile→ BUILD SUCCESS.HiveCatalogTest(and its siblings in this module) fail in@BeforeEach setUpwithIllegalStateException: Hadoop configuration is not available for this CatalogContext(CatalogTestBase.java:126→ResolvingFileIO.configure→CatalogContext.hadoopConf).HadoopUtils.getHadoopConfigurationneedsHADOOP_HOME/HADOOP_CONF_DIRon this machine, andSerializableConfigurationis a no-op stand-in otherwise. This is a pre-existing harness/environment limitation, unrelated to the change — the module's existing tests fail identically on cleanmaster. CI has the Hadoop environment and should run them; I'll report back here once it does.