jerryshao commented on code in PR #13197:
URL: https://github.com/apache/gravitino/pull/13197#discussion_r4071849722
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -585,6 +600,80 @@ private EntityCombinedTable importTable(NameIdentifier
identifier) {
.withHiddenProperties(table.hiddenProperties());
}
+ /**
+ * Tells an external rename apart from a copied id before an import re-binds
a row.
+ *
+ * <p>An import that finds a {@link StringIdentifier} but no row under this
name overwrites the
+ * row that owns the id. That is right after an external rename: the old
name is gone and the row
+ * should follow the table. It is wrong when the id was copied ({@code
CREATE TABLE t2 LIKE t1}
+ * carries {@code TBLPROPERTIES}, so does a copy tool or a restored backup):
the source table is
+ * still there, and re-binding would move its row and every attachment keyed
by that id (owner,
+ * tags, policies, role grants) to the copy. The store cannot tell the two
apart; only the
+ * external catalog can, so this asks it whether the id's current owner
still exists.
+ */
+ private NameIdentifier checkImportedIdNotCopied(NameIdentifier identifier,
long id) {
+ NameIdentifier currentOwner =
findRegisteredTableById(identifier.namespace(), id);
+ if (currentOwner == null || currentOwner.equals(identifier)) {
+ return currentOwner;
+ }
+ NameIdentifier catalogIdent = getCatalogIdentifier(identifier);
+ boolean distinctOwnerStillExists =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithTableOps(
+ ops -> {
+ if (!ops.tableExists(currentOwner)) {
Review Comment:
[Question] When the alias is accepted this returns `false`, so the import
proceeds and `store.put(entity, true)` rewrites the row under the newly
requested spelling. On a backend that resolves aliases without advertising
case-insensitivity — precisely the case this branch exists for — two clients
loading `t` and `T` will rename the entity back and forth: each load takes the
schema write lock, bumps `current_version`, soft-deletes the previous version
row and rewrites the column rows (TableMetaService.insertTable:118-190). Reads
become writes, and the version history grows for as long as the two spellings
keep alternating.
The ping-pong predates this PR — the unconditional re-bind did the same —
but this branch is where the alias case is now deliberately recognised, which
makes it the natural place to stop it. When the only difference is case and the
listing confirms a single object, would it be better to keep the stored name
and skip the import write entirely?
Verified by: read `checkImportedIdNotCopied` (:601-661) and `importTable`
(:529-585), then traced the overwrite path in TableMetaService.insertTable
(:109-190) to confirm what `store.put(.., true)` rewrites.
`testLoadTableAcceptsCaseAlias` asserts id continuity but not that the stored
name flipped.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -549,6 +551,7 @@ private EntityCombinedTable importTable(NameIdentifier
identifier) {
+ "when Table is renamed by external systems not controlled by
Gravitino. In this "
+ "case, we need to overwrite the stored entity to keep the
consistency.",
stringId);
Review Comment:
[Important] This check only looks inside `identifier.namespace()`
(`findRegisteredTableById`, line 649), so the cross-schema copy — `CREATE TABLE
db2.copy LIKE db1.orders`, no less realistic than the same-schema one — returns
early here and falls through to the store guard instead.
That guard throws `EntityAlreadyExistsException` (OccWriteSupport.java:89),
`importTable` rethrows it unchanged (line 579), and `loadTable` catches it at
line 176 as an HA race: it reloads, the copy is still not imported, and the
caller ends up with
> `UnsupportedOperationException: Table managed by multiple catalogs. This
may cause unexpected issues such as privilege conflicts. To resolve: Remove all
catalogs managing this table, then recreate one catalog...`
That diagnosis is wrong for a copied identifier, tells the operator to tear
down catalogs, and never names `gravitino.identifier` or the source table — so
the user-facing behaviour claimed in the PR description ("fails with an
explanatory error") does not hold for exactly the case the store layer was
added for. `SchemaOperationDispatcher.java:277-288` carries the identical
handler and the same outcome for a cross-catalog schema copy.
Two ways out: widen `findRegisteredTableById` to search the catalog rather
than the schema so the good message is produced here, or make the store guard
throw something the handler at line 176 can tell apart from a genuine
concurrent import. Either way this deserves a dispatcher-level test on the
cross-schema copy.
Verified by: read `importTable` (:529-585), `checkImportedIdNotCopied`
(:601-661), `loadTable` (:162-189), `SchemaOperationDispatcher` (:265-290) and
`OccWriteSupport.checkOverwriteIdNotOwnedByOtherParent` (:85-90) on this
branch, then traced the cross-schema path by hand; grepped the tests —
`TestTableMetaService` asserts only the store-level throw, nothing exercises
the dispatcher for it.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalog.java:
##########
@@ -86,7 +86,13 @@ static boolean shouldValidateWarehouseProperty(String
backend, String warehouse)
@Override
public Capability newCapability() {
- return new
IcebergCatalogCapability(HierarchicalSchemaUtil.schemaSeparator());
+ Map<String, String> properties = entity().getProperties();
+ boolean hiveBackend =
+ properties != null
+ && IcebergCatalogBackend.HIVE
+ .name()
+
.equalsIgnoreCase(properties.get(IcebergConstants.CATALOG_BACKEND));
+ return new
IcebergCatalogCapability(HierarchicalSchemaUtil.schemaSeparator(), hiveBackend);
Review Comment:
[Important] This flips `caseSensitiveOnName` for SCHEMA and TABLE on **every
existing** Iceberg-on-Hive catalog, not only for the alias case this PR needs.
`Capability.normalizeName` folds to lowercase (Capability.java:94),
`TableNormalizeDispatcher.loadTable` applies it on every load
(TableNormalizeDispatcher.java:61-64), and the javadoc at Capability.java:81-85
states the normalized name is "persisted as the Gravitino entity name". After
an upgrade, an entity stored as `MyTable` is therefore looked up as `mytable`:
- If the external properties carry a `gravitino.identifier`, the new alias
branch re-binds it and the row is renamed — presumably the intent.
- If they do not (a table created directly in Iceberg and imported earlier
under a mixed-case name), `internalLoadTable` finds no entity (:693-702) and
`importTable` takes the `else` branch, minting a fresh id (:557). The old row
survives under its mixed-case name, and its owner, tags, policies and
statistics stay keyed to the old id — silently orphaned, with two entity rows
for one physical table.
That is the same class of damage this PR sets out to prevent, arriving
through a different door. It is also a user-facing change the PR description
does not mention, and it is separable from the copied-identifier fix. Could it
go in its own PR, with a note on what happens to entities already registered
under mixed-case names?
Verified by: read Capability.java:74-95,
CapabilityHelpers.applyCaseSensitive:147-179, TableNormalizeDispatcher:60-131,
and TableOperationDispatcher.internalLoadTable:690-720 / importTable:529-560 on
this branch.
`TestIcebergCatalogCapability.testBackendSpecificIdentifierNormalization`
covers the wiring but starts from an empty store, so this path is untested.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/OccWriteSupport.java:
##########
@@ -68,6 +68,28 @@ public static <T> T findAndLockForOverwrite(
return current;
}
+ /**
+ * Refuses an overwrite whose stable ID is already owned by a live row under
another parent.
+ *
+ * <p>An import trusts the ID it finds in the external object. When that ID
was copied from
+ * another object (copied table properties, a restored backup), an upsert
keyed by the primary key
+ * would move the existing row, and every attachment keyed by that ID, to
the new name and parent.
+ * The lookup must lock the row so the decision holds until the transaction
ends. A same-parent
+ * match is allowed: that is how an external rename is re-registered.
+ *
+ * @param <T> the persistent object type
+ * @param byIdLockingLookup the locking lookup by stable ID
+ * @param sameParent checks whether the ID owner belongs to the target parent
+ * @throws EntityAlreadyExistsException if the ID belongs to a live row
under another parent
+ */
+ public static <T> void checkOverwriteIdNotOwnedByOtherParent(
+ Supplier<T> byIdLockingLookup, Predicate<T> sameParent) {
+ T owner = byIdLockingLookup.get();
+ if (owner != null && !sameParent.test(owner)) {
+ throw new EntityAlreadyExistsException("The entity ID already belongs to
a different parent");
Review Comment:
[Nit] Two small things on this helper:
- It repeats the second half of `findAndLockForOverwrite` (lines 59-67)
verbatim, message included, so the two copies can drift apart.
`findAndLockForOverwrite` could delegate to this method once its name lookup
misses.
- The message names neither the entity, nor the id, nor the parent.
`PolicyMetaService.java:429` manages "The policy ID %s already belongs to a
different metalake", and this one now surfaces to API users on a plain
`loadTable`/`loadSchema` (see the comment on
TableOperationDispatcher.java:553), where it is the only clue to what happened.
Threading the id and the owning parent into the message would make the
cross-parent case diagnosable from a log line.
Verified by: read OccWriteSupport.java:42-90 on this branch and grepped
`already belongs to a different parent` across `core/src/main` — two hits, both
in this file.
--
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]
