Copilot commented on code in PR #11041:
URL: https://github.com/apache/gravitino/pull/11041#discussion_r3247897252
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -477,6 +477,22 @@ private EntityCombinedTable importTable(NameIdentifier
identifier) {
try {
store.put(tableEntity, true);
} catch (EntityAlreadyExistsException e) {
+ // HA race: another Gravitino node finished importing the same table
between
+ // our internalLoadTable check above and this put. Reuse the existing
entity
+ // instead of failing the caller's load with "managed by multiple
catalogs".
+ TableEntity concurrentTableEntity = getEntity(identifier, TABLE,
TableEntity.class);
+ if (concurrentTableEntity != null) {
+ LOG.info(
+ "Table {} was imported concurrently, reusing the existing entity
in Gravitino.",
+ identifier);
+ return EntityCombinedTable.of(table.tableFromCatalog(),
concurrentTableEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ getCatalogIdentifier(identifier),
+ HasPropertyMetadata::tablePropertiesMetadata,
+ table.tableFromCatalog().properties()));
+ }
Review Comment:
In the EntityAlreadyExistsException fallback, the code unconditionally
treats the conflict as an HA race and reuses whatever entity is currently in
the store. To avoid masking genuine inconsistencies/collisions (e.g., an
existing entity with a different uid than the uid being imported), validate the
fetched entity matches the expected uid (or the parsed StringIdentifier id when
present) before reusing it; otherwise keep the current failure path.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableHookDispatcher.java:
##########
@@ -215,4 +206,47 @@ private void importTableAndSetOwner(
context.userName(),
GravitinoEnv.getInstance().ownerDispatcher());
}
+
+ private void importTableEntity(String catalogName, Namespace namespace,
String tableName) {
+ TableDispatcher tableDispatcher =
GravitinoEnv.getInstance().tableDispatcher();
+ if (tableDispatcher != null) {
+ tableDispatcher.loadTable(
+ IcebergIdentifierUtils.toGravitinoTableIdentifier(
+ metalake, catalogName, TableIdentifier.of(namespace,
tableName)));
+ }
+ }
+
+ private void reconcileTableEntity(
+ IcebergRequestContext context, TableIdentifier tableIdentifier) {
+ // IRC requests can be served by different Gravitino nodes. Without a
distributed TreeLock,
+ // another node may drop or recreate the same Iceberg table between the
backend operation and
+ // this hook's EntityStore mutation. Reconcile the local Gravitino entity
with the Iceberg
+ // backend state to avoid leaving stale/orphan table metadata in
multi-node deployments.
+ if (dispatcher.tableExists(context, tableIdentifier)) {
+ importTableEntity(context.catalogName(), tableIdentifier.namespace(),
tableIdentifier.name());
+ return;
+ }
+
+ deleteTableEntity(context.catalogName(), tableIdentifier);
+
+ if (dispatcher.tableExists(context, tableIdentifier)) {
+ importTableEntity(context.catalogName(), tableIdentifier.namespace(),
tableIdentifier.name());
+ }
+ }
Review Comment:
reconcileTableEntity() can invoke tableDispatcher.loadTable() when the
Iceberg backend reports the table exists. Any runtime failure from loadTable
will now propagate out of dropTable/renameTable even though the backend
operation already succeeded, increasing the chance of client-visible errors and
retries while the system is already in the desired backend state. Consider
making the reconciliation import best-effort here (log and continue) or
otherwise ensuring drop/rename responses aren’t failed solely due to a post-op
metadata reconciliation step.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergViewHookDispatcher.java:
##########
@@ -186,6 +156,12 @@ public void renameView(IcebergRequestContext context,
RenameTableRequest renameV
LOG.error("Failed to rename view entity in store from {} to {}",
sourceIdent, destIdent, ioe);
throw new RuntimeException("Failed to rename view entity in store", ioe);
}
+
+ // IRC rename can race with another node's drop/create on either name.
+ // Reconcile both ends against the Iceberg backend so we don't leave a
+ // stale entity on the source or miss importing a re-created destination.
+ reconcileViewEntity(context, renameViewRequest.source());
+ reconcileViewEntity(context, renameViewRequest.destination());
Review Comment:
reconcileViewEntity() is now called from renameView(). Since
deleteViewEntity() throws RuntimeException on IOException, a transient
EntityStore I/O failure during this post-op reconciliation can cause renameView
to fail even after the Iceberg backend rename has succeeded. Consider treating
reconciliation as best-effort for rename (log and continue) to avoid surfacing
5xx solely due to metadata cleanup.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -387,6 +387,18 @@ private void importSchema(NameIdentifier identifier) {
try {
store.put(schemaEntity, true);
} catch (EntityAlreadyExistsException e) {
+ // HA race: another Gravitino node finished importing the same schema
between
+ // our internalLoadSchema check above and this put. The entity is
already in
+ // the store, so the import is effectively a no-op — let the caller's
load
+ // request succeed instead of failing with "managed by multiple
catalogs".
+ SchemaEntity concurrentSchemaEntity = getEntity(identifier, SCHEMA,
SchemaEntity.class);
+ if (concurrentSchemaEntity != null) {
+ LOG.info(
+ "Schema {} was imported concurrently, reusing the existing entity
in Gravitino.",
+ identifier);
Review Comment:
The EntityAlreadyExistsException handler currently assumes the conflict is
always an HA import race and returns success if any entity exists in the store.
To prevent hiding real conflicts (e.g., an existing schema entity with a
different id than the uid being imported), verify the fetched entity matches
the expected uid / StringIdentifier id before treating it as a concurrent
import; otherwise preserve the existing error path.
--
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]
