jerryshao commented on code in PR #12551:
URL: https://github.com/apache/gravitino/pull/12551#discussion_r3853140856
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -416,6 +417,8 @@ public boolean dropTable(NameIdentifier ident) {
if (droppedFromCatalog) {
try {
store.delete(ident, TABLE);
+ } catch (OptimisticLockException e) {
Review Comment:
**Correctness: concurrent-alter-vs-drop race can permanently orphan the
internal table_meta row.**
Sequence: (1) `doWithCatalog(...).dropTable(ident)` succeeds against the
external catalog (`droppedFromCatalog = true`, external data is now gone); (2)
`store.delete(ident, TABLE)` loses its new version CAS
(`softDeleteTableMetasByTableId` now requires `current_version =
#{currentVersion}`) because another writer concurrently altered the table
between the initial read and this delete, so it throws
`OptimisticLockException`; (3) that exception now propagates straight out of
`dropTable` instead of being retried.
Because the external table is already gone, a client retry of the same
`dropTable` call will have `doWithCatalog(...).dropTable(ident)` report "not
found" (see e.g. `HiveCatalogOperations.dropTable`, which returns `false` when
the table is already absent), so `droppedFromCatalog` becomes `false` on retry
and the `if (droppedFromCatalog) { store.delete(...) }` block — the only place
that ever calls `store.delete` — is skipped entirely. The internal Gravitino
`table_meta` row is then permanently orphaned; no later drop attempt, and no
existing reconciliation job (`OrphanedSchemaCleanup` only targets schemas, not
tables), will ever remove it.
Contrast with `alterTable`, which deliberately swallows the same kind of
conflict via `OperationDispatcher.operateOnEntity` with an explicit
retry-safety rationale ("failing the request would encourage a retry that could
apply the external change twice"). Drop/purge propagate instead, but
retry-safety is actually worse here — it silently blocks all future cleanup of
the internal entity. The same issue applies to `purgeTable` below (line 476).
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -374,4 +381,76 @@ private void
fillTablePOBuilderParentEntityId(TablePO.Builder builder, Namespace
builder.withCatalogId(namespacedEntityId.namespaceIds()[1]);
builder.withSchemaId(namespacedEntityId.entityId());
}
+
+ private TablePO tablePOWithPersistedIdentityAndVersions(TablePO incomingPO,
TablePO persistedPO) {
+ // The upsert derives the version inside the database and may preserve an
existing table ID, so
+ // its dependent rows must carry the identity and versions the database
ended up with.
+ return TablePO.builder(incomingPO)
+ .withTableId(persistedPO.getTableId())
+ .withCurrentVersion(persistedPO.getCurrentVersion())
+ .withLastVersion(persistedPO.getLastVersion())
+ .build();
+ }
+
+ private void deleteTableDependents(TablePO tablePO) {
+ // The table row has already passed its version check. All cleanup below
uses the same database
+ // transaction, so either the table and every related row are deleted
together, or none are.
+ SessionUtils.doWithoutCommit(
+ OwnerMetaMapper.class,
+ mapper ->
+ mapper.softDeleteOwnerRelByMetadataObjectIdAndType(
+ tablePO.getTableId(), MetadataObject.Type.TABLE.name()));
+
TableColumnMetaService.getInstance().deleteColumnsByTableId(tablePO.getTableId());
+ SessionUtils.doWithoutCommit(
+ SecurableObjectMapper.class,
+ mapper ->
+ mapper.softDeleteObjectRelsByMetadataObject(
+ tablePO.getTableId(), MetadataObject.Type.TABLE.name()));
+ SessionUtils.doWithoutCommit(
+ TagMetadataObjectRelMapper.class,
+ mapper ->
+ mapper.softDeleteTagMetadataObjectRelsByMetadataObject(
+ tablePO.getTableId(), MetadataObject.Type.TABLE.name()));
+ SessionUtils.doWithoutCommit(
+ TagMetadataObjectRelMapper.class,
+ mapper ->
mapper.softDeleteTagMetadataObjectRelsByTableId(tablePO.getTableId()));
+ SessionUtils.doWithoutCommit(
+ StatisticMetaMapper.class,
+ mapper -> mapper.softDeleteStatisticsByEntityId(tablePO.getTableId()));
+ SessionUtils.doWithoutCommit(
+ PolicyMetadataObjectRelMapper.class,
+ mapper ->
mapper.softDeletePolicyMetadataObjectRelsByTableId(tablePO.getTableId()));
+ SessionUtils.doWithoutCommit(
+ TableVersionMapper.class,
+ mapper ->
+ mapper.softDeleteTableVersionByTableIdAndVersion(
+ tablePO.getTableId(), tablePO.getCurrentVersion()));
+ }
+
+ private RuntimeException tableWriteFailure(NameIdentifier identifier,
TablePO observedTablePO) {
Review Comment:
**Reuse: this classification algorithm is now duplicated a third time.**
`tableWriteFailure` (lock-and-reread via `selectTableMetaByIdForUpdate`,
compare natural-key fields, return `NoSuchEntityException` or
`ExceptionUtils.concurrentModification`) is essentially a line-for-line copy of
`SchemaMetaService.schemaWriteFailure` and
`CatalogMetaService.catalogWriteFailure` (added by the earlier schema-OCC PR).
No shared helper was extracted, so a future fix to the classification logic
(e.g. adding a field to compare, or changing lock semantics) has to be made
identically in three places — and it already wasn't kept in sync once (the
schema/catalog versions and this one already differ slightly in structure).
Consider extracting one generic `writeFailure(NameIdentifier, T observed,
Function<Long,T> lockingLookup, BiPredicate<T,T> sameParent, EntityType)`
helper that all three services call.
--
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]
