jerryshao commented on code in PR #13771:
URL: https://github.com/apache/gravitino/pull/13771#discussion_r4248188531
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -83,6 +83,49 @@ protected OperationDispatcher(
this.secretManager = secretManager;
}
+ /**
+ * Creates the user-facing error for an external identifier retained by
deleted metadata.
+ *
+ * <p>Restoring that metadata automatically could also restore governance
data that the operator
+ * intended to retire. The error instead gives operators an explicit cleanup
path that imports the
+ * external objects with new identifiers after manual reconciliation.
+ *
+ * @param ident the external entity identifier
+ * @param entityId the stable identifier read from the external system
+ * @param entityType the type of the external entity
+ * @return an exception containing manual reconciliation instructions
+ */
+ protected UnsupportedOperationException
softDeletedExternalIdentifierException(
+ NameIdentifier ident, long entityId, Entity.EntityType entityType) {
+ String cleanupTarget;
+ switch (entityType) {
+ case SCHEMA:
+ cleanupTarget = "the external schema and all of its tables and views";
+ break;
+ case TABLE:
+ cleanupTarget = "the external table";
+ break;
+ case VIEW:
+ cleanupTarget = "the external view";
+ break;
+ default:
+ cleanupTarget = "the external object";
Review Comment:
[Important] This helper covers SCHEMA, TABLE and VIEW, but topic import was
left on the old overwrite path, so the identity reassignment this PR fixes is
still reachable.
`TopicOperationDispatcher.importTopic` builds a `TopicEntity` with `uid =
stringId.id()` read from the external topic and still calls
`store.put(topicEntity, true)` (TopicOperationDispatcher.java:312), under the
same "created externally" rationale the other three dispatchers just dropped
(TopicOperationDispatcher.java:286-291). Topics are securable objects with
their own privileges (AuthorizationUtils.java:116-118,
MetadataObjectUtil.java:266), so an overwrite by stable ID moves the topic row
- and the privilege relations keyed on that ID - to whatever path is being
loaded. That is the failure mode described in #13770, just for a different
entity type.
Either extend the create-only write to `importTopic` (adding a `case TOPIC`
here, e.g. "the external topic") or state in the PR description why topics are
deliberately out of scope.
Verified by: read TopicOperationDispatcher.java:267-316 on this branch;
grepped every `store.put` under core/src/main/java/org/apache/gravitino/catalog
- after this PR only topic import still passes `true` from an import path;
confirmed TOPIC privileges at AuthorizationUtils.java:116-118 and
MetadataObjectUtil.java:266.
##########
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive2IT.java:
##########
@@ -1458,14 +1458,21 @@ public void testOutOfBandRenameKeepsColumnTags() throws
InterruptedException {
return null;
});
- // The column keeps its id, so the tag follows the table to its new name.
+ // Loading the external name now requires manual reconciliation. The
rejected import must
+ // leave the tag relation attached to the original Gravitino identity.
NameIdentifier newIdent = NameIdentifier.of(schema, newName);
- Column column = loadColumn(newIdent, HIVE_COL_NAME1);
- Assertions.assertArrayEquals(new String[] {tagName},
column.supportsTags().listTags());
+ UnsupportedOperationException exception =
+ assertThrows(
+ UnsupportedOperationException.class, () -> loadColumn(newIdent,
HIVE_COL_NAME1));
+ Assertions.assertTrue(
+ exception
+ .getMessage()
+ .contains("does not automatically reconcile external table
renames"));
MetadataObject[] objects =
metalake.getTag(tagName).associatedObjects().objects();
Assertions.assertEquals(1, objects.length);
Assertions.assertEquals(
- String.join(".", catalogName, schema, newName,
HIVE_COL_NAME1).toLowerCase(Locale.ROOT),
+ String.join(".", catalogName, schema, ident.name(), HIVE_COL_NAME1)
Review Comment:
[Important] This inverted assertion locks in dangling governance metadata,
which the PR description does not mention.
After the out-of-band rename, Hive no longer has `ident.name()`, yet the
test now asserts the column tag stays associated with
`catalog.schema.<oldName>.col1`. The new name cannot be loaded either
(assertion at :1464-1470), so the tagged object is unreachable from both sides:
listing it still works (names come from the stored entity), but resolving it
goes through `MetadataObjectUtil.checkMetadataObject` -> `tableExists`
(MetadataObjectUtil.java:257-263 -> TableCatalog.java:91), which now throws
instead of returning false. Before this PR the tag followed the rename and
stayed resolvable.
Two asks: call this out under "user-facing change" (existing tag/privilege
relations are left pointing at a path that exists in neither system), and say
how an operator is expected to re-point or drop them, since the recovery path
in the error message only covers clearing `gravitino.identifier` on the
external object.
Verified by: read CatalogHive2IT.java:1420-1480 on this branch against
origin/main (the test was renamed from `testOutOfBandRenameKeepsColumnTags`);
traced the object-resolution path through MetadataObjectUtil.java:257-263.
##########
core/src/test/java/org/apache/gravitino/catalog/TestSchemaOperationDispatcher.java:
##########
@@ -252,6 +261,30 @@ public void
testConcurrentImportSchemaReusesExistingEntity() throws IOException
Schema loadedSchema = Assertions.assertDoesNotThrow(() ->
dispatcher.loadSchema(schemaIdent));
Assertions.assertEquals(schemaIdent.name(), loadedSchema.name());
Assertions.assertEquals("comment", loadedSchema.comment());
+ verify(entityStore).put(any(SchemaEntity.class), eq(false));
+ }
+
+ @Test
+ public void testImportSchemaWithSoftDeletedIdentifierRequiresManualCleanup()
throws IOException {
+ NameIdentifier schemaIdent =
+ NameIdentifier.of(metalake, catalog, "schema_soft_deleted_identifier");
+ dispatcher.createSchema(schemaIdent, "comment", ImmutableMap.of("k1",
"v1", "k2", "v2"));
+ SchemaEntity schemaEntity = entityStore.get(schemaIdent, SCHEMA,
SchemaEntity.class);
+ entityStore.delete(schemaIdent, SCHEMA);
+
+ reset(entityStore);
+ doThrow(new SoftDeletedEntityIdentifierException(SCHEMA,
schemaEntity.id()))
Review Comment:
[Question] This test stubs `entityStore.put` to throw
`SoftDeletedEntityIdentifierException`, so it proves the dispatcher's message
mapping but not that the backend raises that exception on the import path. The
backend half is proved separately in
`TestSchemaMetaService.testCatalogReregistrationRejectsSoftDeletedIdentifiers`,
which calls `backend.insert` directly. The same split applies to the new table
and view soft-deleted tests.
Nothing joins the two halves: remove catalog -> re-register ->
`loadSchema`/`loadTable` is never exercised. If duplicates ever surface
differently from `insertEntity` (a name conflict instead of a primary-key
conflict, or a wrapped cause), `isSoftDeleted(e.id())` would return false and
the generic multi-catalog message would be shown, while both existing unit
tests keep passing.
Is an integration-level test over the relational store planned for that
flow? It would also give the reviewer's catalog re-registration scenario a
regression guard.
Verified by: read the new tests in TestSchemaOperationDispatcher,
TestTableOperationDispatcher and TestViewOperationDispatcher (all stub
`entityStore.put`), the new `TestSchemaMetaService` template test, and
JDBCBackend.insert:194-206, whose catch block is reached only by the latter.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -177,16 +178,21 @@ public Table loadTable(NameIdentifier ident) throws
NoSuchTableException {
entityCombinedTable =
TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () ->
importTable(ident));
} catch (EntityAlreadyExistsException e) {
- // HA race: another Gravitino node concurrently imported this table.
Reload from the
- // entity store to pick up the entity stored by the winning node.
- LOG.info(
- "Table {} was concurrently imported by another node; reloading
from store.", ident);
+ // A create-only import can conflict either because another Gravitino
node won an HA race,
+ // or because the external identifier is already stored under another
Gravitino path.
+ // Reload to distinguish a successful concurrent import from an
identity conflict.
+ LOG.info("Table {} import conflicted with existing metadata; reloading
from store.", ident);
entityCombinedTable =
TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadTable(ident));
if (!entityCombinedTable.imported()) {
throw new 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 to ensure single-catalog management.");
+ "Table "
+ + ident
+ + " cannot be imported because its name or external
identifier is already "
+ + "managed under a different Gravitino identifier. Gravitino
does not "
+ + "automatically reconcile external table renames or manage
the same table "
+ + "through multiple paths because overwriting its stored
identity can corrupt "
+ + "privilege relationships. Reconcile the external metadata
manually.");
Review Comment:
[Important] This refusal reaches callers that only expect
`NoSuchTableException`, so an identity conflict breaks existence pre-checks
rather than returning false.
`TableCatalog.tableExists`
(api/src/main/java/org/apache/gravitino/rel/TableCatalog.java:91-97) and
`SupportsSchemas.schemaExists`
(api/src/main/java/org/apache/gravitino/SupportsSchemas.java:83-90) implement
existence as "load and catch `NoSuchXException`", so the new
`UnsupportedOperationException` propagates out of a boolean check. Affected
call sites: LoadTableAuthorizationExecutor.java:91 (authorization filter, runs
before the REST handler), MetadataObjectUtil.java:247/257/263 (privilege and
tag object validation), FunctionOperationDispatcher.java:111/176/195 and
SemanticModelOperationDispatcher.java:91/128.
Net effect: once an external rename or multi-path conflict exists, the
operator cannot check existence, and cannot validate or revoke the governance
metadata that the new message asks them to reconcile - and the error surfaces
from an authorization filter instead of the operation they called. Consider
treating the import refusal as "exists" inside the `*Exists` paths, or
confirming the filter maps it to a sane status instead of a 500.
Verified by: read both API default methods and each call site listed on this
branch; the same throw is added in ViewOperationDispatcher (new block at
151-171) and SchemaOperationDispatcher (new block at 286-294), so all three
types are affected.
--
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]
