yuqi1129 commented on code in PR #11362:
URL: https://github.com/apache/gravitino/pull/11362#discussion_r3356571351
##########
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:
The table and view drop paths wrap their orphan-cleanup call in
`IcebergOrphanSchemaCleanup.bestEffortCleanUp`, which adds a
`try/catch(RuntimeException)` so any failure in the two setup calls
(`schemaSeparator()`, `toGravitinoSchemaIdentifier()`) does not propagate back
to the caller.
Here in `dropNamespace`, `SchemaEntityCleaner.deleteOrphanedSchemaEntities`
is called directly inside the `doWithTreeLock` lambda without that guard. Those
setup calls are unlikely to throw in a running server, but for consistency —
and to ensure `dropNamespace` never returns an error when the actual drop
already succeeded — consider wrapping in a try/catch or routing through
`IcebergOrphanSchemaCleanup.bestEffortCleanUp` the same way the table and view
paths do.
##########
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:
`purgeTable` now runs the orphan-schema cleanup unconditionally (good), but
the PR only adds regression tests for `dropTable`. Could you add a
corresponding test for `purgeTable`? The cleanup path is structurally the same,
but a separate test ensures a future regression (e.g. someone re-guarding
`cleanUpOrphanedSchemaEntities` behind `if (droppedFromCatalog)`) would be
caught.
##########
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:
The private `cleanUpOrphanedSchemaEntities` here is identical to the one in
`TableOperationDispatcher` (same parameters, same body, same semantics).
Duplicated logic diverges silently — e.g. if a future PR adjusts `includeSelf`
semantics or error handling in one class and misses the other.
Consider extracting it as a package-private static helper in
`SchemaEntityCleaner` (or promoting the `doWithCatalog` pattern into a shared
base) so both dispatchers share a single implementation.
##########
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:
`SchemaEntityCleaner.deleteOrphanedSchemaEntities` already wraps its entire
body in `catch(Exception)` and swallows all failures, so nothing can escape it.
The outer `catch(RuntimeException)` here can only be reached by the two setup
lines above it (`schemaSeparator()` and `toGravitinoSchemaIdentifier()`), which
are extremely unlikely to throw in a live server.
Given that, this class is mostly a thin wrapper that adds one extra log
entry. Alternatively: move the try/catch directly around the two setup calls,
then let both `IcebergTableHookDispatcher` and `IcebergViewHookDispatcher` call
`SchemaEntityCleaner.deleteOrphanedSchemaEntities` directly — the same pattern
`IcebergNamespaceHookDispatcher.dropNamespace` now uses — and remove this
indirection entirely.
--
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]
