yuqi1129 commented on code in PR #12235:
URL: https://github.com/apache/gravitino/pull/12235#discussion_r3763047917
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -339,20 +339,17 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
return droppedFromCatalog;
}
- // For the unmanaged schema, it could happen that the schema:
- // 1. It's not found in the catalog (dropped directly from
underlying sources)
- // 2. It's found in the catalog but not in the store (not managed by
Gravitino)
- // 3. It's found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the schema from the store,
but we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, SCHEMA, true);
- } catch (NoSuchEntityException e) {
- LOG.warn("The schema to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result can mean that the schema was renamed directly in
the external catalog.
+ // Only remove the stored registration after the catalog confirms
that this drop deleted
+ // the schema and its descendants.
+ if (droppedFromCatalog) {
Review Comment:
Thanks for pointing this out. This thread is on `dropSchema`, but the same
ambiguity exists for tables and views. A `false` result only tells us that the
old name was absent when the external drop ran; it cannot distinguish a true
out-of-band drop from a concurrent or out-of-band rename. Deleting the stored
registration in the latter case irreversibly removes Gravitino-only metadata,
so this PR deliberately preserves it as the safer default.
You are right that a true external drop can therefore leave a stale
registration, and there is currently no generic API to explicitly remove it.
The stale row is not returned by normal `list` or `load` operations because
they consult the external catalog first, but explicit registration cleanup or
reconciliation remains a separate limitation. We should not describe `false` as
meaning that the object was dropped; it means only that this call did not find
the old name to drop.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -374,20 +395,17 @@ public boolean dropTable(NameIdentifier ident) {
return droppedFromCatalog;
}
- // For unmanaged table, it could happen that the table:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the table from the store, but
we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, TABLE);
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result can mean that a concurrent rename already moved
the external table.
+ // Only remove the stored registration after the catalog confirms
that this drop deleted
+ // the table.
+ if (droppedFromCatalog) {
Review Comment:
Yes, the same ambiguity applies to `dropTable` and `purgeTable`. If `false`
is caused by a concurrent rename, deleting the old registration recreates the
metadata-loss race fixed here. If it is caused by a true out-of-band drop,
preserving it can leave stale metadata. Since the boolean result does not let
us distinguish those cases, this PR chooses the non-destructive behavior. The
lack of an explicit cleanup or reconciliation API is a real follow-up
limitation rather than a reason to restore unconditional deletion.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -338,6 +350,15 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
"UPDATE",
tableId);
+ if (isRenameTable && updatedTableEntity == null) {
Review Comment:
`EntityStore.update` is contractually expected to return the updated entity
or throw. `operateOnEntity` converts `NoSuchEntityException`, other store
failures, and an ID mismatch into `null`, so this check means that no valid,
matching updated registration was obtained.
That said, `null` does not identify only one failure mode, and the ID
mismatch is currently detected after the store call. Therefore it is accurate
as a consistency check, but less precise if described narrowly as determining
whether the physical update ran. A stricter rename-specific update path that
validates the expected ID inside the updater would make the semantics clearer.
--
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]
