JingsongLi commented on code in PR #10078:
URL: https://github.com/apache/paimon/pull/10078#discussion_r4254196585
##########
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:
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.
##########
paimon-hive/paimon-hive-catalog/src/main/java/org/apache/paimon/hive/HiveCatalog.java:
##########
@@ -250,14 +251,31 @@ private Pair<Path, Boolean> initialTableLocation(
Path location;
if (tableOptions.containsKey(CoreOptions.PATH.key())) {
externalTable = true;
- location = new Path(tableOptions.get(CoreOptions.PATH.key()));
+ // A `LOCATION '/path'` without a scheme must resolve against the
default filesystem;
+ // otherwise FileIO#get treats it as local and the schema files
land on the driver's
+ // disk on a cluster.
+ location = resolveLocationScheme(new
Path(tableOptions.get(CoreOptions.PATH.key())));
} else {
externalTable = usingExternalTable(tableOptions);
location = getTableLocation(identifier, null);
}
return Pair.of(location, externalTable);
}
+ /**
+ * Returns a location whose scheme is explicitly resolved. A {@code CREATE
TABLE ... LOCATION
+ * '/path'} carries no scheme, and {@link FileIO#get(Path,
CatalogContext)} treats a schemeless
+ * path as a local one - so on a cluster the schema files would be written
to the driver's local
+ * filesystem instead of the configured default filesystem, and the table
would be created
+ * against a location no other engine can read.
+ */
+ private Path resolveLocationScheme(Path location) {
+ if (location.toUri().getScheme() != null) {
+ return location;
+ }
+ return new Path(FileSystem.getDefaultUri(hiveConf).toString(),
location);
Review 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.
##########
paimon-hive/paimon-hive-catalog/src/main/java/org/apache/paimon/hive/HiveCatalog.java:
##########
@@ -1270,15 +1288,122 @@ protected void createTableImpl(Identifier identifier,
Schema schema) {
tableSchema,
location,
externalTable)));
+ } catch (AlreadyExistsException e) {
+ // Another caller registered the same identifier while we were
writing the schema. That
+ // caller won the race and owns the metastore entry (and, for a
managed winner, the
+ // actual data files) - we must not touch it here, otherwise we
would drop a table we do
+ // not own. Only the schema files written by this call are cleaned
up.
+ cleanupFailedCreateSchema(identifier, location, externalTable);
+ throw new RuntimeException(
+ "Table "
+ + identifier.getFullName()
+ + " was concurrently created by another caller.",
+ e);
} 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.
+ *
+ * <p>Cleanup only runs when this call can prove it owns the registered
entry: the table stored
+ * in the metastore must point at the {@code location} this call resolved.
A concurrently
+ * registered table (for example an external create that reused an
existing filesystem schema at
+ * a different location) is left untouched, so its data files are never
deleted.
+ */
+ @VisibleForTesting
+ void cleanupOnCreateTableFailure(Identifier identifier, Path location,
boolean externalTable) {
+ if (!ownsRegisteredTable(identifier, location)) {
+ LOG.info(
+ "Skipping cleanup of [{}]: the registered table is not
owned by this call.",
+ identifier.getFullName());
+ return;
Review 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.
--
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]
