jerryshao commented on code in PR #13771: URL: https://github.com/apache/gravitino/pull/13771#discussion_r4265575164
########## catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiCatalogCapability.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.catalog.lakehouse.hudi; + +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.capability.CapabilityResult; + +/** Describes the capabilities of the Hudi catalog backed by Hive Metastore. */ +public class HudiCatalogCapability implements Capability { + + /** Creates a Hudi catalog capability for the Hive Metastore backend. */ + public HudiCatalogCapability() {} + + @Override + public CapabilityResult caseSensitiveOnName(Scope scope) { + if (scope == Scope.SCHEMA || scope == Scope.TABLE) { + return CapabilityResult.unsupported( + "The Hudi Hive backend is case insensitive for schemas and tables."); Review Comment: [Nit] Hudi-HMS and native Hive views get the same case-folding behaviour change as Iceberg and Paimon, but neither of their catalog docs says so. `docs/lakehouse-iceberg-catalog.md:60-75` and the Paimon equivalent gained an "Identifier Case Behavior" section with the upgrade cleanup steps. Grepping `docs/lakehouse-hudi-catalog.md` and `docs/apache-hive-catalog.md` for "case-insensitive", "case sensitiv" and "casing" returns nothing, so a pre-change Hudi `MyDb.MyTable` registration - or a Hive `MyView` registration, after `HiveCatalogCapability.java:65` started folding VIEW - hits the retained-identifier conflict with no documented recovery, while the identical change for Iceberg and Paimon is documented. Adding the same short section to both docs would close that. Verified by: grep over both doc files on this head; the two capability changes read in this diff. ########## core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java: ########## @@ -177,16 +178,27 @@ public Table loadTable(NameIdentifier ident) throws NoSuchTableException { entityCombinedTable = TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () -> importTable(ident)); } catch (EntityAlreadyExistsException e) { - // HA race: another Gravitino node concurrently imported this table. Reload from the - // entity store to pick up the entity stored by the winning node. - LOG.info( - "Table {} was concurrently imported by another node; reloading from store.", ident); + // A create-only import can conflict either because another Gravitino node won an HA race, + // or because the external identifier is already stored under another Gravitino path. + // Reload to distinguish a successful concurrent import from an identity conflict. + LOG.info("Table {} import conflicted with existing metadata; reloading from store.", ident); entityCombinedTable = TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> internalLoadTable(ident)); if (!entityCombinedTable.imported()) { throw new UnsupportedOperationException( - "Table managed by multiple catalogs. This may cause unexpected issues such as privilege conflicts. " - + "To resolve: Remove all catalogs managing this table, then recreate one catalog to ensure single-catalog management."); + "Table " + + ident + + " cannot be imported because its name or external identifier is already " + + "managed under a different Gravitino identifier. Gravitino does not " + + "automatically reconcile external table renames or stale registrations. " + + "Before retrying, manually resolve the conflict: either restore the external " + + "table name and identifier to match its Gravitino registration, or have an " + + "administrator safely remove the stale Gravitino registration and its " + + "privileges, tags, and other relationships. Do not only remove " + + "`gravitino.identifier` while the conflicting registration remains, because " + + "Gravitino may associate the external table with that registration. After the " + + "stale registration is removed, clear `gravitino.identifier` from the external " + + "table and load it again."); Review Comment: [Important] The recovery this message prescribes cannot be carried out through Gravitino's API for a table or a view, so an operator who hits this error has no documented way out. The message now tells the operator to "have an administrator safely remove the stale Gravitino registration". There is no operation that removes only the registration: - `TableOperationDispatcher.dropTable` deletes the store row only when the external drop succeeded, and the comment at :443-446 says a false result deliberately preserves the registration. `ViewOperationDispatcher.dropView` is identical (:362-374). In the out-of-band rename case this message targets, nothing exists at the stale path, so the drop returns false and the row survives - the instruction cannot be completed and the load keeps failing. - Where the object is still visible at the stale path (a shared HMS, or a rename onto a name that is already registered), `dropTable` reaches the external catalog first, so following the instruction deletes the live object. - Schemas have one escape hatch - `dropSchema(ident, cascade=true)` deletes the row even when the external drop fails (`SchemaOperationDispatcher.java:591-593`) - but it also attempts the external drop first. - For the pre-upgrade mixed-case case that `docs/lakehouse-iceberg-catalog.md:69` gives the same instruction for, the row cannot even be addressed: `TableNormalizeDispatcher.dropTable` folds the identifier before the dispatcher sees it (:105-108), so `dropTable("Orders")` targets `orders` and never the retained `Orders` row. This extends @mchades' PR comment ("Unregistering Gravitino metadata must be distinct from dropping the physical object"), which the new wording now depends on. Either name the mechanism operators actually have (cascading schema drop, or direct metadata-store cleanup) in all three messages and the two docs blocks, or add a metadata-only unregister path. Verified by: reading `dropTable`, `dropView` and `dropSchema` plus `TableNormalizeDispatcher` on this head; no test drives the prescribed recovery. ########## core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java: ########## @@ -147,8 +148,33 @@ public View loadView(NameIdentifier ident) throws NoSuchViewException { NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); schemaDispatcher.loadSchema(schemaIdent); - entityCombinedView = - TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () -> importView(ident)); + try { + entityCombinedView = + TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () -> importView(ident)); + } catch (EntityAlreadyExistsException e) { + // A create-only import can conflict either because another Gravitino node won an HA race, + // or because the external identifier is already stored under another Gravitino path. + // Reload to distinguish a successful concurrent import from an identity conflict. + LOG.info("View {} import conflicted with existing metadata; reloading from store.", ident); + entityCombinedView = + TreeLockUtils.doWithTreeLock(ident, LockType.READ, () -> internalLoadView(ident)); + if (!entityCombinedView.imported()) { + throw new UnsupportedOperationException( + "View " + + ident + + " cannot be imported because its name or external identifier is already " + + "managed under a different Gravitino identifier. Gravitino does not " + + "automatically reconcile external view renames or stale registrations. " + + "Before retrying, manually resolve the conflict: either restore the external " + + "view name and identifier to match its Gravitino registration, or have an " + + "administrator safely remove the stale Gravitino registration and its " + + "privileges, tags, and other relationships. Do not only remove " + + "`gravitino.identifier` while the conflicting registration remains, because " + + "Gravitino may associate the external view with that registration. After the " + + "stale registration is removed, clear `gravitino.identifier` from the external " + + "view and load it again."); Review Comment: [Nit] These 13 lines are now the schema and table messages with one noun swapped, in a third place. `SchemaOperationDispatcher.java:287-301` and `TableOperationDispatcher.java:189-201` carry character-for-character the same text apart from "view"/"table"/"schema" and the trailing cleanup target, and the three dispatcher unit tests assert the same three substrings on all three. `OperationDispatcher.softDeletedExternalIdentifierException` (:97-126) already solves exactly this with an `EntityType` switch for the noun; a sibling helper for the retained-identifier message would keep the three from drifting the next time the wording is tuned - this is the second commit in a row that edits all three in lockstep. Verified by: diffed the three blocks against each other and read the existing helper. -- 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]
