yuqi1129 commented on code in PR #13728:
URL: https://github.com/apache/gravitino/pull/13728#discussion_r4401615088
##########
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:
Re-scoped in 851a1802f7 as preparatory work, as suggested. The title and
description now explicitly state that no production connector removes
identifiers in this PR and that #13475 remains open. Removed the Fixes claim.
The callback is opt-in so the existing Iceberg resource cleanup and other
connectors retain their previous behavior. Actual identity-checked
property/comment cleanup remains part of #13475.
##########
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:
Addressed by making the preparation scope explicit and correcting the test
claims in 851a1802f7. The manager test is now named for callback
discovery/failure behavior, not identifier removal. Added listing failure,
callback failure, unsupported connector, and legacy callback cases, asserting
successful metadata deletion and cache invalidation. Actual identifier removal,
user metadata preservation, and fresh-ID reimport regressions remain required
by #13475; this PR does not claim to cover them. The related suites passed 63
tests; removing opt-in gating and propagating callback errors makes three
regression cases fail.
##########
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:
Fixed in 851a1802f7. External schemas are listed only after confirming
CatalogDropAware and an explicit supportsExternalIdentifierCleanup() opt-in.
The default is false, so even legacy resource-only callbacks such as Iceberg
retain their no-query behavior; Hive/JDBC do not enter discovery either. Added
tests for unsupported and legacy connectors doing no discovery. The callback
contract and PR description explicitly state that best-effort failure handling
does not bound I/O latency or shorten the existing lock lifetime.
##########
.codegraph/.gitignore:
##########
@@ -0,0 +1,16 @@
+# CodeGraph data files
Review Comment:
Confirmed absent from the final cumulative diff in 851a1802f7. This PR
contains only CatalogManager, CatalogDropAware, and their focused tests; no
CodeGraph files.
--
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]
