jerryshao commented on code in PR #13771:
URL: https://github.com/apache/gravitino/pull/13771#discussion_r4255232645
##########
core/src/main/java/org/apache/gravitino/catalog/TopicOperationDispatcher.java:
##########
@@ -309,6 +309,10 @@ private void importTopic(NameIdentifier identifier) {
.build();
try {
+ // Kafka does not persist a removable Gravitino identifier. Its
immutable topic UUID is
+ // converted to the entity ID on every load, so catalog re-registration
must rebind that ID
+ // to the current Gravitino path. Overwrite is safe for this
Kafka-specific identity model:
+ // Kafka does not support topic rename, and deleting a topic gives a
replacement a new UUID.
Review Comment:
[Important] Both claims in this comment are true, but neither covers the
case this PR exists to fix: one Kafka cluster behind two Gravitino paths.
`KafkaCatalogOperations` derives the entity ID from the Kafka topic UUID
alone (`convertToGravitinoId`, `KafkaCatalogOperations.java:567`, used at
`:237` and `:284`); nothing scopes it to a catalog or metalake, and nothing
stops two Kafka catalogs - in one metalake or in two - from pointing at the
same `bootstrap.servers`. Loading the topic through the second path reaches
`insertTopicMetaOnDuplicateKeyUpdate` (`TopicMetaService.java:84`), whose `ON
DUPLICATE KEY UPDATE` rewrites `topic_name`, `metalake_id`, `catalog_id` and
`schema_id` on the existing row
(`provider/base/TopicMetaBaseSQLProvider.java:74-79`). The topic entity, and
the TOPIC privileges keyed on its ID (`AuthorizationUtils.java:116-118`,
`MetadataObjectUtil.java:266`), move to whichever path was loaded last. That is
the #13770 failure mode, for topics, and it is reachable without any rename or
recreate.
The re-registration argument on @mchades' thread is sound - the ID is not
removable, so create-only would wedge Kafka catalogs. But the fix can be
narrower than an unconditional overwrite: rebind only when the existing row is
soft-deleted (the state re-registration actually produces) and refuse when it
is live under another path. If that is more than this PR should carry, the
comment and the "user-facing change" section should name the multi-path
exposure as a known limitation rather than call overwrite safe.
Verified by: read `TopicOperationDispatcher.java:267-320` and
`TopicMetaService.insertTopic:64-95` on this branch; read the upsert SQL in
`provider/base/TopicMetaBaseSQLProvider.java:55-84`; `grep -n
convertToGravitinoId` in `KafkaCatalogOperations.java` shows no catalog or
metalake component; `JDBCBackend.isSoftDeleted:1037-1049` has no TOPIC case, so
topics also get no soft-deleted diagnosis.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/SparkCommonIT.java:
##########
@@ -630,7 +630,7 @@ protected void testAlterTableReplaceColumns() {
tableInfo = getTableInfo(tableName);
checkTableColumns(tableName, updateColumns, tableInfo);
- sql(String.format("INSERT INTO %S VALUES(3, 'name2', 10)", tableName));
+ sql(String.format("INSERT INTO %s VALUES(3, 'name2', 10)", tableName));
Review Comment:
[Important] Lowercasing this statement removes the only coverage of
mixed-case identifier resolution, while the behaviour it exposed is still
unfixed.
`%S` has been in this shared base test since before this PR and passes on
`main`. It fails on this branch for the Hive-backed Iceberg suite because
`IcebergCatalogCapability` has no `caseSensitiveOnName` override, so
`TEST_REPLACE_COLUMNS_TABLE` reaches the dispatcher un-normalized; Iceberg's
HiveCatalog resolves it to the lowercase Hive table along with its
`gravitino.identifier`; the create-only write at
`TableOperationDispatcher.java:598` then conflicts with the live row, the
reload at `:185` still finds nothing under the uppercase path, and `loadTable`
throws the manual-cleanup error (`:188-196`). On `main` the same load silently
renamed the stored row, which is the bug this PR fixes - so the refusal is
intended, but it also means ordinary mixed-case SQL against a case-insensitive
backend now fails at load.
386e00b fixed that with the capability override; 03272ce reverted the
override (correctly, per the two open threads on it) and this line is what took
its place. With it, nothing in the suite exercises the uppercase path, so the
regression is invisible rather than resolved. Either normalize identifiers for
backends that fold case and keep `%S` here, or keep `%S` and record the new
behaviour in a per-suite override so it is visible and attributable.
Verified by: `git show origin/main:...SparkCommonIT.java` shows `%S` at this
line on main; `git diff --name-only origin/main...HEAD` no longer lists
`IcebergCatalogCapability.java`; `grep -rn caseSensitiveOnName api/ catalogs/
core/` finds implementations only for Hive and Glue; read
`TableOperationDispatcher.java:166-197` and `544-607` on this branch.
##########
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive2IT.java:
##########
@@ -1458,20 +1460,81 @@ public void testOutOfBandRenameKeepsColumnTags() throws
InterruptedException {
return null;
});
- // The column keeps its id, so the tag follows the table to its new name.
+ // Loading the external name now requires manual identifier cleanup. The
rejected import must
+ // leave the tag relation attached to the original Gravitino identity.
NameIdentifier newIdent = NameIdentifier.of(schema, newName);
- Column column = loadColumn(newIdent, HIVE_COL_NAME1);
- Assertions.assertArrayEquals(new String[] {tagName},
column.supportsTags().listTags());
+ UnsupportedOperationException exception =
+ assertThrows(
+ UnsupportedOperationException.class, () -> loadColumn(newIdent,
HIVE_COL_NAME1));
+
Assertions.assertTrue(exception.getMessage().contains("`gravitino.identifier`"));
MetadataObject[] objects =
metalake.getTag(tagName).associatedObjects().objects();
Assertions.assertEquals(1, objects.length);
Assertions.assertEquals(
- String.join(".", catalogName, schema, newName,
HIVE_COL_NAME1).toLowerCase(Locale.ROOT),
+ String.join(".", catalogName, schema, ident.name(), HIVE_COL_NAME1)
+ .toLowerCase(Locale.ROOT),
objects[0].fullName().toLowerCase(Locale.ROOT));
} finally {
metalake.deleteTag(tagName);
}
}
+ @Test
+ public void testCatalogReregistrationRequiresExternalIdentifierCleanup()
Review Comment:
[Question] This closes the schema half of the layering gap from the last
pass - thanks. Does anything cover the table half?
The schema registered here is created empty, so exactly one retained ID is
exercised. In a real re-registration every table and view under the dropped
catalog also keeps its own `gravitino.identifier`, and
`TableMetaService.isSoftDeleted:117` / `ViewMetaService.isSoftDeleted` go
through the same `JDBCBackend.isSoftDeleted:1037-1049` branch. So after the
operator clears the schema's identifier and the schema imports with a new ID,
each `loadTable` should fail again with its own soft-deleted error until that
table's identifier is cleared too. That is exactly what the schema message asks
for - "the external schema and all of its tables and views"
(`OperationDispatcher.java:102`) - but nothing demonstrates the sequence, and
on an HMS schema with many tables it is the part an operator will spend their
time on.
Creating one table in the schema before the drop and asserting load -> clear
-> load for it as well looks cheap here, and would pin the per-object cleanup
contract that the error message promises.
Verified by: read the new test in full at `CatalogHive2IT.java:1481-1537`
(`createSchema` with `Collections.emptyMap()` and no table creation); read
`JDBCBackend.isSoftDeleted:1037-1049`, `TableMetaService.isSoftDeleted:111-121`
and `OperationDispatcher.softDeletedExternalIdentifierException:96-124` on this
branch.
--
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]
