jerryshao commented on code in PR #13771:
URL: https://github.com/apache/gravitino/pull/13771#discussion_r4256897069
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalog.java:
##########
@@ -86,7 +86,10 @@ static boolean shouldValidateWarehouseProperty(String
backend, String warehouse)
@Override
public Capability newCapability() {
- return new
IcebergCatalogCapability(HierarchicalSchemaUtil.schemaSeparator());
+ String backend =
entity().getProperties().get(IcebergConstants.CATALOG_BACKEND);
+ return new IcebergCatalogCapability(
+ HierarchicalSchemaUtil.schemaSeparator(),
+ IcebergCatalogBackend.HIVE.name().equalsIgnoreCase(backend));
Review Comment:
[Important] The Hive-fold flag is read from the Gravitino catalog's own
`catalog-backend`, so an Iceberg catalog with `backend=rest` in front of a
Hive-backed REST service is still left case-sensitive and the uppercase path
still fails.
`IcebergCatalogBackend.HIVE.name().equalsIgnoreCase(backend)` is only true
for `catalog-backend=hive`. It is not true for `rest`, yet `rest` is routinely
a Hive catalog one hop away - that is exactly what
`SparkIcebergCatalogRestBackendIT` sets up: the Gravitino catalog gets
`catalog-backend=rest` pointing at the aux Iceberg REST service
(`SparkIcebergCatalogRestBackendIT.java:38-45`), and
`SparkEnvIT.initIcebergRestServiceEnv` configures that service with
`catalog-backend=hive` plus `hiveMetastoreUri` (`SparkEnvIT.java:254-287`,
taken because `useDynamicIcebergRestConfigProvider()` defaults to false at
`SparkEnvIT.java:94`). The class javadoc says so itself: "the real backend
catalog is HiveCatalog".
That suite inherits `supportsReplaceColumns() == true`
(`SparkIcebergCatalogIT.java:108`), so the two uppercase statements at
`SparkCommonIT.java:633-634` run there, go through Gravitino's `loadTable`
(spark `BaseCatalog.java:272`, then `tableExists` for `IF NOT EXISTS`), resolve
to the lowercase Hive table together with its `gravitino.identifier`, and hit
the create-only write at `TableOperationDispatcher.java:598`. The reload under
the uppercase path still finds nothing, so `loadTable` throws the
manual-cleanup error. `SparkIcebergCatalogRestRoutingIT` is fine, because it
keeps `catalog-backend=hive` on the Gravitino side
(`SparkIcebergCatalogRestRoutingIT.java:51-62`).
Either decide folding from the behaviour of the catalog that actually
resolves the name (or restore a case-insensitive lookup / `resolveTableName`
for folding backends, which 25297d4 removed), or keep `%S` out of the shared
base test and record the REST-backed-by-Hive case as a known gap. Note CI has
not exercised this yet: every SparkIT run on `ca27d8c` is still queued.
Verified by: read `IcebergCatalog.java:88-93`,
`IcebergCatalogCapability.java:62-70`, `SparkCommonIT.java:607-645`,
`SparkIcebergCatalogRestBackendIT.java` and `SparkEnvIT.java:94-107,254-287` on
this branch; `git show origin/main:...SparkCommonIT.java` has `%S` at that line
and that suite is green on main, which is what shows the uppercase name does
resolve below Gravitino; read spark `BaseCatalog.loadTable:271-297`;
`get_check_runs` on this head shows SparkIT queued, not passed.
##########
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)) {
+ return CapabilityResult.unsupported(
+ "The Iceberg Hive backend is case insensitive for schemas and
tables.");
+ }
+ return Capability.super.caseSensitiveOnName(scope);
+ }
Review Comment:
[Important] `Scope.VIEW` is left out of the fold, so Hive-backed Iceberg
views keep the exact failure this block fixes for tables.
`ViewNormalizeDispatcher` normalizes with `Capability.Scope.VIEW`
(`ViewNormalizeDispatcher.java:105-117`; the scope exists at
`Capability.java:40`), so with `hiveBackend == true` a view identifier is still
passed through case-sensitively. Iceberg's `HiveCatalog` keeps views in the
same Hive metastore namespace as tables (`IcebergViewCatalogOperations`), where
names fold, so loading a view through an uppercase spelling resolves to the
lowercase view and its external `gravitino.identifier`, while the stored-entity
lookup under the uppercase path misses. The create-only write this PR adds at
`ViewOperationDispatcher.java:605` then conflicts and the refusal at
`ViewOperationDispatcher.java:155-170` fires - the same sequence as the table
case, for a path with no test at all.
Please either add `Scope.VIEW` next to SCHEMA and TABLE (and to
`TestIcebergCatalogCapability.testHiveBackendFoldsSchemaAndTableNamesOnly`,
which currently asserts only the three scopes), or say in the comment why views
are deliberately excluded.
Verified by: read this block in full plus the new
`TestIcebergCatalogCapability` tests on this branch; read
`ViewNormalizeDispatcher.java:105-117` and `Capability.Scope` at
`Capability.java:37-48`; read `ViewOperationDispatcher.java:140-175,595-615`;
confirmed Iceberg view support at `IcebergViewCatalogOperations.java`. The one
step I did not execute is the metastore fold for views; it is the same HMS
object namespace the table case relies on.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/SparkCommonIT.java:
##########
@@ -630,6 +630,7 @@ protected void testAlterTableReplaceColumns() {
tableInfo = getTableInfo(tableName);
checkTableColumns(tableName, updateColumns, tableInfo);
+ sql(String.format("CREATE TABLE IF NOT EXISTS %S (id int)", tableName));
Review Comment:
[Nit] This `IF NOT EXISTS` declares a one-column schema for a three-column
table, which turns a failure here into a confusing error somewhere else.
`test_replace_columns_table` has `id`, `name2`, `age` at this point. On any
backend where the uppercase spelling does not resolve, `IF NOT EXISTS` is not a
no-op: it creates a second, one-column table, and the next line then fails with
an arity mismatch on `VALUES(3, 'name2', 10)` (or the `SELECT` at :635 returns
one row and trips the `assertEquals(2, ...)`), instead of reporting the
identifier conflict this line exists to guard. Declaring the real column list,
or asserting `getTableInfo` on the uppercase name, keeps the failure pointing
at the right cause.
Verified by: read `testAlterTableReplaceColumns` in full
(`SparkCommonIT.java:607-645`) - `createSimpleTable` plus the `REPLACE COLUMNS`
at :620-630 leave three columns before this statement.
##########
core/src/test/java/org/apache/gravitino/catalog/TestTopicOperationDispatcher.java:
##########
@@ -166,14 +169,65 @@ public void testCreateAndLoadTopic() throws IOException {
AuditInfo.builder().withCreator("gravitino").withCreateTime(Instant.now()).build())
.build();
doReturn(unmatchedEntity).when(entityStore).get(any(),
eq(Entity.EntityType.TOPIC), any());
- Topic loadedTopic4 = topicOperationDispatcher.loadTopic(topicIdent1);
- // Succeed to import the topic entity
+ UnsupportedOperationException exception =
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () -> topicOperationDispatcher.loadTopic(topicIdent1));
+ Assertions.assertTrue(exception.getMessage().contains("immutable Kafka
topic UUID"));
+ verify(entityStore).put(any(TopicEntity.class), eq(false));
+ verify(entityStore).restoreSoftDeletedEntity(any(TopicEntity.class));
reset(entityStore);
TopicEntity topicEntity =
entityStore.get(topicIdent1, Entity.EntityType.TOPIC,
TopicEntity.class);
Assertions.assertEquals("test", topicEntity.auditInfo().creator());
- // Audit info is gotten from the catalog, not from the entity store
- Assertions.assertEquals("test", loadedTopic4.auditInfo().creator());
+ }
+
+ @Test
+ public void testConcurrentImportTopicReusesExistingEntity() throws
IOException {
+ Namespace topicNs = Namespace.of(metalake, catalog,
"schema_topic_concurrent_import");
+ NameIdentifier schemaIdent = NameIdentifier.of(topicNs.levels());
+ schemaOperationDispatcher.createSchema(
+ schemaIdent, "comment", ImmutableMap.of("k1", "v1", "k2", "v2"));
+ NameIdentifier topicIdent = NameIdentifier.of(topicNs,
"topic_concurrent_import");
+ createTopicForLockTest(topicIdent);
+ TopicEntity importedTopicEntity = entityStore.get(topicIdent, TOPIC,
TopicEntity.class);
+
+ reset(entityStore);
+ doThrow(new NoSuchEntityException("mock error"))
+ .doThrow(new NoSuchEntityException("mock error"))
+ .doReturn(importedTopicEntity)
+ .when(entityStore)
+ .get(any(), eq(TOPIC), any());
+ doThrow(new EntityAlreadyExistsException("mock conflict"))
+ .when(entityStore)
+ .put(any(TopicEntity.class), eq(false));
+
+ Topic loadedTopic =
+ Assertions.assertDoesNotThrow(() ->
topicOperationDispatcher.loadTopic(topicIdent));
+ Assertions.assertEquals(topicIdent.name(), loadedTopic.name());
+ verify(entityStore).restoreSoftDeletedEntity(any(TopicEntity.class));
Review Comment:
[Question] All three new topic dispatcher tests stub `entityStore`, so
nothing joins the dispatcher and backend halves for topics the way
`CatalogHive2IT` now does for schemas and tables. Is a Kafka-side test planned?
Here `put(..., false)` is stubbed to throw and `restoreSoftDeletedEntity` is
stubbed to return true or left at its default; the backend half is proved
separately by `TestTopicMetaService.testRestoreSoftDeletedTopicOnly`, which
calls `backend.restoreSoftDeletedEntity` directly. Nothing exercises drop Kafka
catalog -> re-register -> `loadTopic`, and no test was added under
`catalogs/catalog-kafka`.
The specific thing that would pin: the restore is only attempted for
`EntityAlreadyExistsException` (`TopicOperationDispatcher.java:321-335`). A
retained `topic_id` reaches that type only because
`TopicMetaService.insertTopic` routes the duplicate-key `PersistenceException`
through `ExceptionUtils.checkSQLException`. If that ever surfaced differently -
a wrapped cause, or a `SoftDeletedEntityIdentifierException` once
`JDBCBackend.isSoftDeleted` gains a TOPIC case - the restore would be skipped
and a plain catalog re-registration would report "managed under another active
Gravitino path", which is the one message that is wrong for that state. Both
existing unit tests would keep passing.
Verified by: read the three new tests at
`TestTopicOperationDispatcher.java:172-230`,
`TestTopicMetaService.testRestoreSoftDeletedTopicOnly:93-114`,
`TopicOperationDispatcher.loadTopic:100-130` and `importTopic:276-346`,
`TopicMetaService.insertTopic:64-95` and `restoreSoftDeletedTopic:98-129`, and
`JDBCBackend.insert:194-206` plus `isSoftDeleted:1037-1049` (still no TOPIC
case) 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]
