roryqi commented on code in PR #13771:
URL: https://github.com/apache/gravitino/pull/13771#discussion_r4248663278
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -665,7 +677,9 @@ private void importSchema(NameIdentifier identifier) {
.build())
.build();
try {
- store.put(schemaEntity, true);
+ // Never overwrite imported schema metadata. In particular, an overwrite
by stable ID can
+ // silently change catalog ownership and make existing privileges refer
to the wrong path.
+ store.put(schemaEntity, false);
Review Comment:
Addressed in 386e00b914. A retained ID from a soft-deleted registration is
now distinguished with SoftDeletedEntityIdentifierException. We intentionally
do not restore or reuse that deleted identity; the user-facing error asks the
operator to remove gravitino.identifier from the external objects and retry so
they are imported with new IDs. I also added a real Hive catalog
drop/re-register/load regression test and extended the relational-store
coverage. No governance reconciliation is performed because relation cleanup
already happens with entity deletion.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -582,11 +592,12 @@ private EntityCombinedTable importTable(NameIdentifier
identifier) {
.withAuditInfo(audit)
.build();
try {
- // Overwrite only with the id stored in the catalog: it identifies the
table, so a row under
- // the same name with another id is stale and gets replaced. A generated
id identifies
- // nothing, so a row that appeared meanwhile, e.g. from a concurrent
import on another node,
- // must win. The plain insert then conflicts, and loadTable reloads that
row.
- store.put(tableEntity, stringId != null);
+ // Never overwrite imported table metadata. In particular, an overwrite
by stable ID can
+ // silently change schema or catalog ownership and make existing
privileges refer to the
+ // wrong path.
+ store.put(tableEntity, false);
Review Comment:
Addressed in 386e00b914. IcebergCatalogCapability now follows Hive backend
case-insensitive behavior for schema, table, and column names. I removed the
expected-failure override, so the inherited test keeps its original success
assertions. SparkIcebergCatalogHiveBackendIT35.testAlterTableReplaceColumns
passed locally.
##########
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:
Addressed in 386e00b914. Topic import now uses a create-only store write,
reloads after a conflict to preserve the HA-race behavior, and reports an
explicit identifier-cleanup error for a real identity conflict. TOPIC is also
included in soft-deleted-ID detection, with dispatcher and relational-store
regression coverage.
##########
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:
I rechecked the API contract and kept the existing semantics. The default
tableExists/schemaExists/viewExists implementations are load plus catch
NoSuchXException; other load failures are intentionally propagated. An
identifier conflict means the object cannot be loaded correctly, so returning
either true or false would hide that failure. Therefore the conflict remains
explicit in both load and exists paths. The message no longer asks callers to
reconcile governance data.
##########
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:
I removed governance reconciliation from the implementation and error text.
Relations are automatically removed when the stale Gravitino registration is
deleted, so this change does not add any relation re-pointing logic. The
out-of-band rename test remains only as a no-silent-overwrite guard: cleanup is
manual by deleting any stale registration and clearing the external
gravitino.identifier before importing the object with a new ID.
##########
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:
Added in 386e00b914.
CatalogHive2IT.testCatalogReregistrationRequiresExternalIdentifierCleanup now
exercises the complete flow with the relational store and Hive backend: create
an external schema, drop the catalog, re-register it, verify loadSchema reports
the retained soft-deleted ID, remove the external gravitino.identifier, and
verify the schema imports successfully with a new ID. The exact Docker-backed
test passed locally.
--
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]
