jerryshao commented on code in PR #13771:
URL: https://github.com/apache/gravitino/pull/13771#discussion_r4266735087
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -83,6 +83,90 @@ protected OperationDispatcher(
this.secretManager = secretManager;
}
+ /**
+ * Creates the user-facing error for an external identifier retained by
deleted metadata.
+ *
+ * <p>The error gives operators an explicit cleanup path that imports the
external objects with
+ * new identifiers.
+ *
+ * @param ident the external entity identifier
+ * @param entityId the stable identifier read from the external system
+ * @param entityType the type of the external entity
+ * @return an exception containing manual cleanup instructions
+ */
+ protected UnsupportedOperationException
softDeletedExternalIdentifierException(
+ NameIdentifier ident, long entityId, Entity.EntityType entityType) {
+ String cleanupTarget;
+ switch (entityType) {
+ case SCHEMA:
+ cleanupTarget = "the external schema and all of its tables and views";
+ break;
+ case TABLE:
+ cleanupTarget = "the external table";
+ break;
+ case VIEW:
+ cleanupTarget = "the external view";
+ break;
+ default:
+ cleanupTarget = "the external object";
+ }
+
+ return new UnsupportedOperationException(
+ String.format(
+ "%s %s cannot be imported because external identifier %s is
retained by a "
+ + "soft-deleted Gravitino registration. "
+ + "Gravitino does not automatically restore or reuse deleted
registrations. "
+ + "Remove `%s` from %s before retrying. The objects will be
imported with new "
+ + "identifiers.",
+ entityType.name().toLowerCase(),
+ ident,
+ entityId,
+ StringIdentifier.ID_KEY,
+ cleanupTarget));
+ }
+
+ /**
+ * Creates the user-facing error for an external identifier conflict with
active metadata.
+ *
+ * <p>The error describes the supported recovery choices without implying
that Gravitino offers
+ * object-level metadata-only unregister operations.
+ *
+ * @param ident the external entity identifier
+ * @param entityType the type of the external entity
+ * @return an exception containing conflict recovery instructions
+ */
+ protected UnsupportedOperationException
retainedExternalIdentifierConflictException(
+ NameIdentifier ident, Entity.EntityType entityType) {
+ String entityName;
+ switch (entityType) {
+ case SCHEMA:
+ entityName = "schema";
+ break;
+ case TABLE:
+ entityName = "table";
+ break;
+ case VIEW:
+ entityName = "view";
+ break;
+ default:
+ entityName = "object";
+ }
+
+ return new UnsupportedOperationException(
+ String.format(
+ "%s %s cannot be imported because its name or external identifier
is already managed "
Review Comment:
[Important] The PR description now contradicts the recovery procedure this
commit ships.
The message built here tells the operator that Gravitino `does not support
metadata-only unregister of an individual external` object and that `Recovery
requires an administrator to force-drop and recreate the containing Gravitino
catalog registration`, losing every registration, privilege and tag in that
catalog. All four doc sections added in 4d0b464/782c1e2 say the same (for
example `docs/apache-hive-catalog.md:86-98`).
The PR's "Does this PR introduce any user-facing change?" section still
describes the previous, per-object remedy: that the load "asks the operator to
delete the stale Gravitino registration and remove `gravitino.identifier` from
the external object before importing it with a new ID". That is exactly the
operation this message now says is unsupported, and it understates the blast
radius by a whole catalog. The description is what reviewers and release notes
read; please bring it in line with the message and the docs - catalog-wide
force-drop, then clear `gravitino.identifier`, then re-register and reapply
governance.
Verified by: read the final message text at OperationDispatcher.java:155-167
on this head, the four doc sections added by 4d0b464 and 782c1e2, and the PR
body as currently published.
##########
docs/lakehouse-iceberg-catalog.md:
##########
@@ -57,6 +57,26 @@ If you are using the Gravitino with Trino, you can pass the
Trino Iceberg connec
If you are using the Gravitino with Spark, you can pass the Spark Iceberg
connector configuration using prefix `spark.bypass.`. For example, using
`spark.bypass.io-impl` to pass the `io-impl` to the Spark Iceberg connector in
Spark runtime.
+#### Identifier Case Behavior
+
+For `catalog-backend=hive`, schema, table, and view identifiers are
case-insensitive; you can use any casing.
+
+For `catalog-backend=rest`, always use the same identifier casing that was
used when the object was first registered. Loading the same object with
different casing is rejected.
Review Comment:
[Nit] These two sentences cover `hive` and `rest` but leave out the other
backends, and the code treats them like `rest`, not like `hive`.
`IcebergCatalogBackend` also has `JDBC`, `MEMORY` and `CUSTOM`
(`IcebergCatalogBackend.java:21-27`), and `IcebergCatalog.newCapability` folds
only when the property equals `hive` (`IcebergCatalog.java:89-93`). The same
gap is in `docs/lakehouse-paimon-catalog.md:85-87`, where
`PaimonCatalogBackend` also has `FILESYSTEM` and `JDBC`
(`PaimonCatalogBackend.java:22-27`).
A reader on `catalog-backend=jdbc` finds no statement at all, even though
they get the exact-case behaviour and the same rejection on a different casing.
Suggest phrasing it as `hive` versus "every other backend".
Verified by: read both backend enums and both `newCapability()`
implementations on this head.
##########
catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiCatalog.java:
##########
@@ -54,6 +55,11 @@ protected CatalogOperations newOps(Map<String, String>
config) {
return new HudiCatalogOperations();
}
+ @Override
+ public Capability newCapability() {
Review Comment:
[Nit] Two small asymmetries with the Iceberg and Paimon equivalents in the
same commit:
1. No unit test asserts this wiring. `TestIcebergCatalog:133` and
`TestPaimonCatalog:133` each gained a
`newCapability().caseSensitiveOnName(...)` assertion, but `TestHudiCatalog` has
none, so the only thing proving `HudiCatalog` actually returns
`HudiCatalogCapability` rather than `Capability.DEFAULT` is the Docker-tagged
`HudiCatalogHMSIT`. A two-line assertion in `TestHudiCatalog` would cover it in
the fast suite.
2. `HudiCatalogCapability` folds unconditionally while the other two key on
the `catalog-backend` property. That is correct today - `BackendType` has only
`HMS` (`BackendType.java:21-22`) - but the capability's own javadoc says
"backed by Hive Metastore", so a future non-HMS backend would silently inherit
the fold. Keying it on `BackendType` here, as `PaimonCatalog.java:73-76` does,
would make that impossible to get wrong.
Verified by: read `HudiCatalogPropertiesMetadata`, `BackendType`, all three
`newCapability()` implementations, and `TestHudiCatalog` on this head.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/iceberg/SparkIcebergCatalogRestBackendIT.java:
##########
@@ -33,6 +35,17 @@
@DisabledIf("org.apache.gravitino.integration.test.util.ITUtils#isEmbedded")
public abstract class SparkIcebergCatalogRestBackendIT extends
SparkIcebergCatalogIT {
+ /**
+ * Tests replacing columns without assuming the case-sensitivity of the
catalog behind the REST
+ * service. Gravitino cannot infer that backend capability through the REST
protocol.
+ */
+ @Override
+ @Test
+ @EnabledIf("supportsReplaceColumns")
+ protected void testAlterTableReplaceColumns() {
Review Comment:
[Question] Was `SparkIcebergCatalogRestRoutingIT` meant to get this same
override?
`runAlterTableReplaceColumnsTest(true)` is the new default
(`SparkCommonIT.java:610-620`), and this is the only subclass that opts out.
`SparkIcebergCatalogRestRoutingIT extends SparkIcebergCatalogIT` directly
(`SparkIcebergCatalogRestRoutingIT.java:43`), not this class, so it inherits
the uppercase statements.
In routing mode Spark speaks the Iceberg REST protocol to Gravitino's
auxiliary service, and those requests are served by
`IcebergTableOperationExecutor` through `IcebergCatalogWrapperManager`
(`IcebergTableOperationExecutor.java:58-62,173-182`) - they never reach
`TableOperationDispatcher`/`TableNormalizeDispatcher`, so the new
`IcebergCatalogCapability` fold has no effect on that path. If that test goes
green it will be because Iceberg's `HiveCatalog` and HMS fold the name
themselves, not because of this PR; if HMS did not fold, it would fail exactly
as the REST-backend case does. Either outcome is worth deciding deliberately:
add the same `runAlterTableReplaceColumnsTest(false)` override here if the REST
protocol is meant to stay exact-case end to end, or say in the test's javadoc
why the routed path is expected to tolerate uppercase.
No SparkIT job has run on this head (all queued), so CI has not answered
this yet.
Verified by: read the class hierarchy of all `SparkCommonIT` subclasses and
their `supportsReplaceColumns()` (only hive and the three iceberg suites return
true), and traced the REST-routed load path to `IcebergTableOperationExecutor`.
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -83,6 +83,90 @@ protected OperationDispatcher(
this.secretManager = secretManager;
}
+ /**
+ * Creates the user-facing error for an external identifier retained by
deleted metadata.
+ *
+ * <p>The error gives operators an explicit cleanup path that imports the
external objects with
+ * new identifiers.
+ *
+ * @param ident the external entity identifier
+ * @param entityId the stable identifier read from the external system
+ * @param entityType the type of the external entity
+ * @return an exception containing manual cleanup instructions
+ */
+ protected UnsupportedOperationException
softDeletedExternalIdentifierException(
+ NameIdentifier ident, long entityId, Entity.EntityType entityType) {
+ String cleanupTarget;
+ switch (entityType) {
+ case SCHEMA:
+ cleanupTarget = "the external schema and all of its tables and views";
+ break;
+ case TABLE:
+ cleanupTarget = "the external table";
+ break;
+ case VIEW:
+ cleanupTarget = "the external view";
+ break;
+ default:
+ cleanupTarget = "the external object";
+ }
+
+ return new UnsupportedOperationException(
+ String.format(
+ "%s %s cannot be imported because external identifier %s is
retained by a "
+ + "soft-deleted Gravitino registration. "
+ + "Gravitino does not automatically restore or reuse deleted
registrations. "
+ + "Remove `%s` from %s before retrying. The objects will be
imported with new "
+ + "identifiers.",
+ entityType.name().toLowerCase(),
+ ident,
+ entityId,
+ StringIdentifier.ID_KEY,
+ cleanupTarget));
+ }
+
+ /**
+ * Creates the user-facing error for an external identifier conflict with
active metadata.
+ *
+ * <p>The error describes the supported recovery choices without implying
that Gravitino offers
+ * object-level metadata-only unregister operations.
+ *
+ * @param ident the external entity identifier
+ * @param entityType the type of the external entity
+ * @return an exception containing conflict recovery instructions
+ */
+ protected UnsupportedOperationException
retainedExternalIdentifierConflictException(
+ NameIdentifier ident, Entity.EntityType entityType) {
+ String entityName;
+ switch (entityType) {
+ case SCHEMA:
+ entityName = "schema";
+ break;
+ case TABLE:
+ entityName = "table";
+ break;
+ case VIEW:
+ entityName = "view";
+ break;
+ default:
+ entityName = "object";
+ }
+
+ return new UnsupportedOperationException(
+ String.format(
+ "%s %s cannot be imported because its name or external identifier
is already managed "
+ + "under a different Gravitino identifier. Gravitino does not
automatically "
+ + "reconcile external %s renames or stale registrations, and
it does not support "
+ + "metadata-only unregister of an individual external %s.
Recovery requires an "
+ + "administrator to force-drop and recreate the containing
Gravitino catalog "
+ + "registration. This catalog-wide reset removes every stored
registration and "
+ + "its privileges, tags, and other relationships in that
Gravitino catalog. "
+ + "Follow the catalog-specific recovery guidance to confirm
that external objects "
+ + "are retained, and do not remove `%s` until the conflicting
catalog registration "
Review Comment:
[Nit] Three small things in this message, now that it is the single
user-facing text for all three entity types:
1. It never states the step the operator must take - it only says when *not*
to take it (`do not remove ... until the conflicting catalog registration has
been dropped`). The docs do say to remove `gravitino.identifier` after the
drop; the message should too, or point at the doc page.
2. `the containing Gravitino catalog registration` (line 161) and `the
conflicting catalog registration` (line 165) read as two different catalogs. In
the shared-metastore case they genuinely are: the stale registration lives in
another catalog, and force-dropping the catalog you are loading through fixes
nothing. Naming which one would save an operator a wrong drop.
3. `%s` at the start resolves to the lowercase entity name, so the sentence
now begins `schema metalake.cat.sch cannot be imported` where it previously
began `Schema ...`.
Verified by: read the format string and its five arguments at
OperationDispatcher.java:155-167, and compared with the procedure in
docs/apache-hive-catalog.md:86-98.
--
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]
