geyanggang commented on code in PR #13481:
URL: https://github.com/apache/gravitino/pull/13481#discussion_r4090661406
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -129,4 +131,32 @@ private NameIdentifier
normalizeNameIdentifier(NameIdentifier tableIdent) {
Capability capability = getCapability(tableIdent, catalogManager);
return applyCapabilities(tableIdent, Capability.Scope.TABLE, capability);
}
+
+ /**
+ * Maps a normalized table identifier to the identifier under which the
table is physically stored
+ * by the catalog's backend, via {@link
org.apache.gravitino.rel.TableCatalog#resolveTableName}.
+ *
+ * <p>For catalogs that do not override that hook (the default), this
returns {@code ident}
+ * unchanged without touching the backend. Resolving here — before the
identifier is handed to the
+ * downstream dispatcher — ensures the same physical identifier drives both
the underlying catalog
+ * operation and the Gravitino entity store key, so the two never diverge.
+ */
+ private NameIdentifier resolvePhysicalName(NameIdentifier ident) {
Review Comment:
Race: resolution now runs inside TableOperationDispatcher, as the first step
within the same doWithTreeLock scope each operation already holds, so resolve +
catalog call + entity-store key are atomic on the resolved name. load resolves
under its READ lock; drop/purge/alter under the schema/table WRITE lock. (I
confirmed TreeLockNode uses ReentrantReadWriteLock, so wrapping an outer lock
at the normalize layer would deadlock on a read→write upgrade for the WRITE ops
— doing it inside the existing lock avoids that.)
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -129,4 +131,32 @@ private NameIdentifier
normalizeNameIdentifier(NameIdentifier tableIdent) {
Capability capability = getCapability(tableIdent, catalogManager);
return applyCapabilities(tableIdent, Capability.Scope.TABLE, capability);
}
+
+ /**
+ * Maps a normalized table identifier to the identifier under which the
table is physically stored
+ * by the catalog's backend, via {@link
org.apache.gravitino.rel.TableCatalog#resolveTableName}.
+ *
+ * <p>For catalogs that do not override that hook (the default), this
returns {@code ident}
+ * unchanged without touching the backend. Resolving here — before the
identifier is handed to the
+ * downstream dispatcher — ensures the same physical identifier drives both
the underlying catalog
+ * operation and the Gravitino entity store key, so the two never diverge.
+ */
+ private NameIdentifier resolvePhysicalName(NameIdentifier ident) {
Review Comment:
Contract: the resolver returns the normalized ident (never throws) when the
table is absent, so tableExists/dropTable keep their boolean not-found
semantics. This is in the SPI Javadoc and covered by a test.
--
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]
