yuqi1129 commented on code in PR #13728:
URL: https://github.com/apache/gravitino/pull/13728#discussion_r4229892407
##########
core/src/main/java/org/apache/gravitino/connector/CatalogDropAware.java:
##########
@@ -26,4 +28,24 @@ public interface CatalogDropAware {
/** Performs cleanup after the catalog metadata has been permanently
dropped. */
void onCatalogDropped();
+
+ /**
+ * Performs cleanup after the catalog metadata has been permanently dropped,
when the catalog
+ * registration is unmanaged and its external objects (schemas and the
tables inside them) are
+ * left in place. Implementations should remove the Gravitino identifier
that was written into
+ * those external objects, for example the {@code gravitino.identifier}
property or the "From
+ * Gravitino, DO NOT EDIT" snippet in comments, so that a later catalog that
manages the same
+ * external objects does not inherit stale identifiers.
+ *
+ * <p>Cleanup is best-effort: a failure to clear an identifier must not fail
the drop. The default
+ * implementation only forwards to {@link #onCatalogDropped()}; unmanaged
catalogs that never
+ * write identifiers to external objects do not need to override this method.
+ *
+ * @param remainingExternalSchemas the identifiers of the schemas that still
exist in the external
+ * system after the catalog registration was dropped. The list is empty
for managed catalogs,
+ * whose external objects are dropped together with the registration.
+ */
+ default void onCatalogDropped(List<NameIdentifier> remainingExternalSchemas)
{
+ onCatalogDropped();
Review Comment:
I traced the production implementations to confirm this: no connector
overrides the new overload. The only production implementation of
CatalogDropAware is IcebergCatalogOperations, whose no-arg callback only
removes and closes the in-memory catalog via
IcebergCatalogUtil.removeMemoryCatalog(). HiveCatalogOperations and
JdbcCatalogOperations do not implement CatalogDropAware, so their cleanup
callback is skipped entirely.
Consequently, identifiers stored in external schema/table properties or
comments remain unchanged. Could we add the actual connector cleanup
implementations before claiming Fixes #13475? If this PR is intended only to
introduce the extension point, please describe it as preparatory work and leave
#13475 open.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1417,11 +1417,19 @@ public boolean dropCatalog(NameIdentifier ident,
boolean force)
boolean deleted = store.delete(ident, EntityType.CATALOG, true);
if (deleted) {
markLocalMutation(ident);
+ // Unmanaged catalogs keep their external schemas and tables.
Those objects still
+ // carry the Gravitino identifier written at create/import time,
so a catalog that
+ // later manages them again would inherit stale ownership (see
issue #13475). Collect
+ // the schemas that remain in the external system and let the
connector strip the
+ // identifiers; managed catalogs already dropped their external
objects above, so the
+ // list stays empty.
+ List<NameIdentifier> remainingExternalSchemas =
+ managedStorage ? Collections.emptyList() :
listExternalSchemas(catalogWrapper);
Review Comment:
[Nit] listExternalSchemas() is called before checking whether the connector
implements CatalogDropAware. This adds an external schema-listing call to every
successful unmanaged catalog drop, including Hive/JDBC catalogs that cannot
consume the callback. The call also runs under the metalake write lock after
the metadata deletion has committed; catching exceptions does not avoid waiting
for a slow external system.
Could we check whether the connector supports identifier cleanup before
listing schemas, so connectors without a cleanup implementation retain the
existing drop behavior?
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -1409,7 +1410,118 @@ void testDropCatalogInvalidatesCacheAfterStoreDelete()
throws Exception {
Assertions.assertTrue(dropped);
Assertions.assertFalse(entityStore.exists(ident, EntityType.CATALOG));
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident));
- Mockito.verify((CatalogDropAware) operations).onCatalogDropped();
+ // The drop hands the connector the schemas that remain externally (empty
here: the wrapper's
+ // schema operations are stubbed to return nothing), so residual
identifiers can be cleared.
+ Mockito.verify((CatalogDropAware) operations)
+ .onCatalogDropped(Mockito.eq(Collections.emptyList()));
+ }
+
+ @Test
+ void testDropUnmanagedCatalogCleansResidualExternalIdentifiers() throws
Exception {
+ // Regression test for #13475: dropping an unmanaged catalog must tell the
connector which
+ // external schemas remain, so it can clear the Gravitino identifiers
those objects still carry.
+ NameIdentifier ident = NameIdentifier.of("metalake",
"residual_identifier_drop_test");
+ Map<String, String> props =
+ ImmutableMap.of(
+ "provider",
+ "test",
+ PROPERTY_KEY1,
+ "value1",
+ PROPERTY_KEY2,
+ "value2",
+ PROPERTY_KEY5_PREFIX + "1",
+ "value3");
+
+ catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
"comment", props);
+ Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
+ BaseCatalog<?> catalog =
catalogManager.loadCatalogAndWrap(ident).catalog();
+
+ NameIdentifier externalSchema = NameIdentifier.of("metalake",
ident.name(), "ext_schema");
+ CatalogManager.CatalogWrapper catalogWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Capability capability = Mockito.mock(Capability.class);
+ CapabilityResult unsupportedResult = CapabilityResult.unsupported("Not
managed");
+ Mockito.when(catalogWrapper.tryAcquire()).thenReturn(true);
+ Mockito.doReturn(catalog).when(catalogWrapper).catalog();
+ Mockito.doReturn(capability).when(catalogWrapper).capabilities();
+ Mockito.doReturn(unsupportedResult).when(capability).managedStorage(any());
+ // The external system still holds the schema after the registration is
dropped.
+ Mockito.doReturn(new NameIdentifier[] {externalSchema})
+ .when(catalogWrapper)
+ .doWithSchemaOps(any());
+
+ CatalogOperations operations =
+ Mockito.mock(
+ CatalogOperations.class,
+ Mockito.withSettings().extraInterfaces(CatalogDropAware.class));
+ Mockito.doAnswer(
+ invocation -> {
+ ThrowableFunction<CatalogOperations, ?> function =
invocation.getArgument(0);
+ return function.apply(operations);
+ })
+ .when(catalogWrapper)
+ .doWithCatalogOps(any());
+
+ catalogManager.getCatalogCache().put(ident, catalogWrapper);
+ boolean dropped = catalogManager.dropCatalog(ident, true);
+
+ Assertions.assertTrue(dropped);
+ Assertions.assertFalse(entityStore.exists(ident, EntityType.CATALOG));
+ // The connector is handed the schemas that remain externally so it can
strip the identifiers.
+ Mockito.verify((CatalogDropAware) operations)
+
.onCatalogDropped(Mockito.eq(Collections.singletonList(externalSchema)));
Review Comment:
[Important] This test verifies that the schema list is passed to a mocked
callback, but does not verify that any residual identifier is removed. It
passes even when the connector performs no cleanup, as is currently the case
for the production implementations.
Please add regression coverage using a real cleanup implementation (with a
controlled external backend if needed): assert that schema and table
identifiers are removed from properties/comments while user metadata is
preserved, and that recreating the catalog does not reuse the old IDs. Please
also cover the advertised best-effort behavior when listing or cleaning
external objects fails.
##########
.codegraph/.gitignore:
##########
@@ -0,0 +1,16 @@
+# CodeGraph data files
Review Comment:
[Nit] This local CodeGraph configuration appears unrelated to the catalog
identifier cleanup. Could we move it to a separate change?
--
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]
