jerryshao commented on code in PR #13771:
URL: https://github.com/apache/gravitino/pull/13771#discussion_r4257595573
##########
catalogs/catalog-kafka/src/test/java/org/apache/gravitino/catalog/kafka/integration/test/CatalogKafkaIT.java:
##########
@@ -192,6 +192,35 @@ public void testCatalog() throws ExecutionException,
InterruptedException {
Assertions.assertFalse(adminClient.listTopics().names().get().isEmpty());
}
+ @Test
+ public void testCatalogReregistrationLoadsExistingTopic()
+ throws ExecutionException, InterruptedException {
+ String catalogName =
GravitinoITUtils.genRandomName("test_catalog_reregistration");
+ String topicName =
GravitinoITUtils.genRandomName("test_topic_reregistration");
+ NameIdentifier topicIdent = NameIdentifier.of(DEFAULT_SCHEMA_NAME,
topicName);
+ Map<String, String> properties = ImmutableMap.of(BOOTSTRAP_SERVERS,
kafkaBootstrapServers);
+
+ adminClient.createTopics(ImmutableList.of(new NewTopic(topicName, 1,
(short) 1))).all().get();
+
+ Catalog originalCatalog = createCatalog(catalogName, "original catalog",
properties);
+ Assertions.assertEquals(
+ topicName,
originalCatalog.asTopicCatalog().loadTopic(topicIdent).name());
+
+ Assertions.assertTrue(metalake.dropCatalog(catalogName, true));
+ Assertions.assertTrue(topicExistsInKafka(topicName));
+
+ Catalog reregisteredCatalog = createCatalog(catalogName, "re-registered
catalog", properties);
+ Topic restoredTopic =
+ Assertions.assertDoesNotThrow(
+ () -> reregisteredCatalog.asTopicCatalog().loadTopic(topicIdent));
+ Assertions.assertEquals(topicName, restoredTopic.name());
+ checkTopicReadWrite(topicName);
Review Comment:
[Question] This joins the two topic halves for the restore case - thanks. Is
the refusal case covered anywhere outside the stubbed unit test?
What this drives is the `deleted_at > 0` branch: the cascade soft-deletes
the topic row (`CatalogMetaService.java:314-315` via `store.delete(ident,
CATALOG, true)` at `CatalogManager.java:1417`), so the re-registered catalog's
`loadTopic` hits the retained `topic_id` and `restoreSoftDeletedTopic` rebinds
it. Good.
The other half of the new behaviour is the explicit refusal when the same
immutable Kafka UUID is live under another Gravitino path - the multi-path
exposure this change exists to close, since `convertToGravitinoId` derives the
id from the topic UUID with no catalog or metalake component. That path is only
asserted with `entityStore` stubbed in `TestTopicOperationDispatcher`. Here it
would be cheap: create a second Kafka catalog against the same
`bootstrap.servers` while the first is still live, and assert `loadTopic` on
the second one refuses rather than moving the row. Without it, nothing shows
that an active row is not rebound.
Two smaller things on this test: it does not assert the restored topic's id
equals the pre-drop one (deterministic from the UUID either way, so the
assertion would not distinguish restore from fresh insert - but asserting that
the governance relation survives would), and `checkTopicReadWrite` exercises
Kafka rather than the store, so the only store-level assertion is that
`loadTopic` did not throw.
Verified by: read this test in full (`CatalogKafkaIT.java:195-222`) plus the
existing `dropCatalog(name, true)` at :184 and the helpers at :537, :600, :607,
:660; read `CatalogManager.dropCatalog:1358-1420` and the cascade in
`CatalogMetaService.java:285-335`; read the three stubbed tests at
`TestTopicOperationDispatcher.java:172-230`. Not executed - no Docker test run
in this pass.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/SparkCommonIT.java:
##########
@@ -630,7 +630,8 @@ protected void testAlterTableReplaceColumns() {
tableInfo = getTableInfo(tableName);
checkTableColumns(tableName, updateColumns, tableInfo);
- sql(String.format("INSERT INTO %S VALUES(3, 'name2', 10)", tableName));
+ sql(String.format("CREATE TABLE IF NOT EXISTS %s (id int, name2 string,
age long)", tableName));
+ sql(String.format("INSERT INTO %s VALUES(3, 'name2', 10)", tableName));
Review Comment:
[Important] This drops the last mixed-case coverage from the shared base
test, for the suites that are not the new Hive-backend Iceberg one, and the
statement added in its place cannot fail.
On `main` this line is `INSERT INTO %S` (`git show
origin/main:...SparkCommonIT.java` at 633), and `testAlterTableReplaceColumns`
is enabled for `SparkHiveCatalogIT` (:105-107) and every
`SparkIcebergCatalogIT` subclass (:108-110): the direct-Hive backend,
`SparkIcebergCatalogRestBackendIT` and `SparkIcebergCatalogRestRoutingIT`, each
in v3.5 and v4.0. All of them pass `%S` on `main`. After this edit none of them
exercises an uppercase identifier: the Hive catalog (which folds via
`HiveCatalogCapability`) loses coverage it did not need to lose, and the two
REST suites lose the only test that would have shown the REST-backed-by-Hive
gap you decided to accept.
The replacement `CREATE TABLE IF NOT EXISTS %s (...)` is a no-op in this
shared test - same lowercase name as the table created at :611, so
`tableExists` is trivially true on every backend and the statement can never
fail. It was added to exercise `tableExists` through an uppercase spelling, and
with `%s` it does not.
Suggest keeping `%S` on both statements here and overriding this test in the
suites where the uppercase path is a known gap (the two REST-backed Iceberg
suites), rather than lowercasing the shared base. That keeps the Hive-catalog
and direct-Hive-Iceberg coverage, and makes the REST gap visible and
attributable instead of silent. The new
`SparkIcebergCatalogHiveBackendIT.testCaseInsensitiveTableNameAfterReplaceColumns`
covers one of the four suites.
Verified by: diffed this file against `origin/main`; read
`testAlterTableReplaceColumns` in full (:607-645) and `supportsReplaceColumns`
in all six overrides; `grep -rn "class SparkIcebergCatalog.*extends"` for the
suite hierarchy; read the new test at
`SparkIcebergCatalogHiveBackendIT.java:37-55`.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogCapability.java:
##########
@@ -48,6 +60,15 @@ public CapabilityResult supportsHierarchicalSchema() {
return CapabilityResult.SUPPORTED;
}
+ @Override
+ public CapabilityResult caseSensitiveOnName(Scope scope) {
+ if (hiveBackend && (scope == Scope.SCHEMA || scope == Scope.TABLE || scope
== Scope.VIEW)) {
+ return CapabilityResult.unsupported(
+ "The Iceberg Hive backend is case insensitive for schemas, tables,
and views.");
Review Comment:
[Important] Thanks for adding VIEW here. The same fold is still missing from
the native Hive catalog, where this PR's new create-only view write makes the
identical failure reachable.
`HiveCatalogCapability.caseSensitiveOnName` folds only SCHEMA, TABLE and
COLUMN (`HiveCatalogCapability.java:61-72`), so `Scope.VIEW` stays
case-sensitive for `catalog-hive` - and `catalog-hive` does expose views
(`HiveCatalogOperations.java:100` implements `ViewCatalog`, delegating to
`HiveViewCatalogOperations`). A view created through Gravitino carries
`gravitino.identifier` in its HMS table parameters, because
`internalCreateView` injects the `StringIdentifier` into the properties handed
to the catalog ops (`ViewOperationDispatcher.java:397-402`) and the Hive view
ops round-trip properties as HMS parameters.
So loading that view through an uppercase spelling:
`ViewNormalizeDispatcher` leaves the name alone, HMS folds it and returns the
view with its stored `gravitino.identifier`, the stored-entity lookup under the
uppercase path misses (`internalLoadView` ... `withImported(viewEntity !=
null)`), `importView` reads the external id and the new create-only write at
`ViewOperationDispatcher.java:605` conflicts, so `loadView` throws the
manual-cleanup error (`ViewOperationDispatcher.java:152-170`). On `main` that
same load silently reassigned the row - the bug this PR fixes - so the refusal
is intended, but it leaves ordinary mixed-case view access against HMS broken
for the Hive catalog in exactly the way you just fixed for Iceberg-on-Hive.
Adding `case VIEW:` next to SCHEMA/TABLE/COLUMN in `HiveCatalogCapability`
looks like the same one-line change. If Hive views are deliberately out of
scope, saying so in this comment or the PR description would be enough.
Verified by: read this method and `HiveCatalogCapability.java:60-72` on this
branch; confirmed `catalog-hive` implements `ViewCatalog` at
`HiveCatalogOperations.java:100` and `HiveViewCatalogOperations.java:62`; read
`ViewOperationDispatcher.java:140-175`, `397-402` and `548-612`, and confirmed
`git diff origin/main...HEAD` changes line 605 from `store.put(viewEntity,
true)` to `false`; `grep -rn caseSensitiveOnName` over api/core/catalogs shows
overrides only in Hive, Glue and Iceberg. The one step I did not execute is the
HMS round-trip for a view name; it is the same TBLS namespace the table case in
this PR relies on.
--
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]
