roryqi commented on code in PR #11362:
URL: https://github.com/apache/gravitino/pull/11362#discussion_r3357164382
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -420,10 +424,33 @@ public boolean purgeTable(NameIdentifier ident) throws
UnsupportedOperationExcep
} catch (Exception e) {
throw new RuntimeException(e);
}
+ // Run unconditionally: an out-of-band purge may have left orphaned
schema entities. The
+ // cleanup is best-effort and stops as soon as a schema still exists.
+ cleanUpOrphanedSchemaEntities(catalogIdent, schemaIdentifier);
Review Comment:
Done. Added `testPurgeMissingTableCleansUpSchemas`, mirroring the
`dropTable` out-of-band case: the catalog has already removed the table and
auto-dropped the empty namespaces, so `purgeTable` returns false but the
orphaned schema entities must still be cleaned up. It also guards against a
future regression that re-guards the cleanup behind the catalog drop result.
(Required adding a `purgeTable` override to `TestCatalogOperations`, since the
default interface method throws `UnsupportedOperationException`.)
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceHookDispatcher.java:
##########
@@ -147,49 +144,26 @@ public void dropNamespace(IcebergRequestContext context,
Namespace namespace) {
() -> {
dispatcher.dropNamespace(context, namespace);
- // Only the leaf namespace is dropped from the Iceberg catalog
above. getAncestorNames
- // returns ancestors outermost-to-innermost, so we walk it in
reverse (innermost outward),
- // advancing outermostStale past every ancestor whose Iceberg
namespace no longer exists
- // and stopping at the first ancestor that still exists.
outermostStale therefore ends up
- // as the outermost missing ancestor; everything from there down to
the leaf is now stale
- // in Gravitino.
- //
- // An inner Iceberg namespace cannot exist unless all of its outer
ancestors do, so an
- // ancestor missing from the catalog implies none of its descendants
exist there
- // either. A single cascade delete of the outermost empty namespace
therefore removes
- // it and every descendant Gravitino entity (the leaf plus the
intermediate ancestors)
- // in one batched operation, rather than issuing one delete per
target.
+ // Only the leaf namespace is dropped from the Iceberg catalog
above. Dropping it may have
+ // also emptied its ancestors, so clean up every Gravitino schema
entity whose Iceberg
+ // namespace no longer exists. An inner Iceberg namespace cannot
exist unless all of its
+ // outer ancestors do, so the cleaner cascade-deletes the outermost
stale namespace and
+ // every descendant entity in one batched operation.
//
// Ancestor entities are still only cleaned up when the underlying
Iceberg catalog
// removes the empty parents on leaf-drop, which is
catalog-implementation-dependent.
// For catalogs that keep empty parents, operators may need to drop
them manually.
String separator = HierarchicalSchemaUtil.schemaSeparator();
- Namespace outermostStale = namespace;
- String namespaceName = String.join(separator, namespace.levels());
- List<String> ancestorNames =
- HierarchicalSchemaUtil.getAncestorNames(namespaceName,
separator);
- for (int i = ancestorNames.size() - 1; i >= 0; i--) {
- Namespace ancestor =
Namespace.of(ancestorNames.get(i).split(Pattern.quote(separator)));
- if (dispatcher.namespaceExists(context, ancestor)) {
- break;
- }
- outermostStale = ancestor;
- }
-
- EntityStore store = GravitinoEnv.getInstance().entityStore();
- if (store != null) {
- try {
- store.delete(
- IcebergIdentifierUtils.toGravitinoSchemaIdentifier(
- metalake, catalogName, outermostStale, separator),
- Entity.EntityType.SCHEMA,
- true);
- } catch (NoSuchEntityException ignore) {
- // Already gone.
- } catch (IOException ioe) {
- throw new RuntimeException("io exception when deleting schema
entity", ioe);
- }
- }
+ SchemaEntityCleaner.deleteOrphanedSchemaEntities(
Review Comment:
Fixed. `dropNamespace` now routes through
`IcebergOrphanSchemaCleanup.bestEffortCleanUp`, the same guarded entry point as
the table and view paths, so all three Iceberg hook drop paths share one
implementation and the cleanup can never surface an error after the backend
drop has already succeeded.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergOrphanSchemaCleanup.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.iceberg.service.dispatcher;
+
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.iceberg.common.utils.IcebergIdentifierUtils;
+import org.apache.gravitino.listener.api.event.IcebergRequestContext;
+import org.apache.gravitino.utils.HierarchicalSchemaUtil;
+import org.apache.gravitino.utils.SchemaEntityCleaner;
+import org.apache.iceberg.catalog.Namespace;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Shared best-effort cleanup of Gravitino schema entities that become
orphaned when an Iceberg
+ * table or view drop empties their backing namespaces.
+ */
+final class IcebergOrphanSchemaCleanup {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(IcebergOrphanSchemaCleanup.class);
+
+ private IcebergOrphanSchemaCleanup() {}
+
+ /**
+ * Removes Gravitino schema entities whose backing Iceberg namespace no
longer exists after a
+ * table or view drop.
+ *
+ * <p>Best-effort: the primary drop has already succeeded, so any failure
here is logged and
+ * swallowed rather than propagated to the caller.
+ *
+ * @param metalake the metalake the catalog belongs to
+ * @param namespaceDispatcher dispatcher used to probe whether a namespace
still exists in the
+ * catalog
+ * @param context the Iceberg request context carrying the catalog name
+ * @param namespace the namespace of the dropped table or view
+ */
+ static void bestEffortCleanUp(
+ String metalake,
+ IcebergNamespaceOperationDispatcher namespaceDispatcher,
+ IcebergRequestContext context,
+ Namespace namespace) {
+ try {
+ String separator = HierarchicalSchemaUtil.schemaSeparator();
+ SchemaEntityCleaner.deleteOrphanedSchemaEntities(
+ GravitinoEnv.getInstance().entityStore(),
+ IcebergIdentifierUtils.toGravitinoSchemaIdentifier(
+ metalake, context.catalogName(), namespace, separator),
+ true,
+ schemaIdent ->
+ namespaceDispatcher.namespaceExists(
+ context,
+ Namespace.of(
+
HierarchicalSchemaUtil.splitSchemaName(schemaIdent.name(), separator))));
+ } catch (RuntimeException e) {
Review Comment:
Addressed by unifying instead of removing: `dropNamespace` now also goes
through `bestEffortCleanUp`, so all three Iceberg hook paths share this single
helper rather than each inlining the setup + cleanup. That gives the wrapper a
clear reason to exist (one guarded place that builds the identifier and
swallows the unlikely setup-call failures) and removes the inconsistency in the
namespace path.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -292,10 +293,33 @@ public boolean dropView(NameIdentifier ident) {
} catch (Exception e) {
throw new RuntimeException(e);
}
+ // Run unconditionally: an out-of-band drop may have left orphaned
schema entities. The
+ // cleanup is best-effort and stops as soon as a schema still exists.
+ cleanUpOrphanedSchemaEntities(catalogIdent, schemaIdentifier);
return droppedFromCatalog;
});
}
+ /**
+ * Removes Gravitino schema entities whose backing catalog schema no longer
exists, starting from
+ * {@code schemaIdentifier} and walking toward its outermost ancestor.
+ *
+ * @param catalogIdent the identifier of the catalog owning the schema
+ * @param schemaIdentifier the schema identifier to start the orphan check
from
+ */
+ private void cleanUpOrphanedSchemaEntities(
Review Comment:
Done. Extracted the duplicated logic into a shared package-private helper
`OrphanedSchemaCleanup` in the `catalog` package; both dispatchers now call
`OrphanedSchemaCleanup.cleanUp(this, ...)`. I kept it out of
`OperationDispatcher` itself, but the helper reuses the dispatcher's
`protected` `store` and `doWithCatalog`, so there's now a single implementation.
--
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]
