yuqi1129 commented on code in PR #12551:
URL: https://github.com/apache/gravitino/pull/12551#discussion_r3851305474
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -219,6 +220,9 @@ protected <R extends HasIdentifier> R operateOnEntity(
R ret = null;
try {
ret = fn.apply(ident);
+ } catch (OptimisticLockException e) {
Review Comment:
Good catch. The external catalog mutation happens before this mirror write,
so propagating a conflict here could make a client repeat a non-idempotent
external alter. Commit 1d874dbbf6 restores operateOnEntity as best effort for
unmanaged external synchronization. Managed operations bypass this helper, so
their TableMetaService OptimisticLockException still reaches the client. Rename
keeps its existing explicit consistency failure because an identity move cannot
be silently ignored.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -139,15 +131,27 @@ public void insertTable(TableEntity tableEntity, boolean
overwrite) throws IOExc
SessionUtils.doWithoutCommit(
TableMetaMapper.class,
mapper -> {
- tablePORef.set(po);
ops.insertPO(mapper, po, overwrite);
+ if (overwrite) {
+ TablePO storedPO =
mapper.selectTableMetaByIdForUpdate(po.getTableId());
Review Comment:
Fixed in 1d874dbbf6. After the upsert we now read the row through the
natural key and carry the persisted table ID plus current and last versions
into table_version_info and column writes. This covers MySQL choosing the
schema/name unique key and preserving the winner ID. The read remains
intentional because the monotonic version is derived atomically by the database
and dependent rows need that exact value; deriving it client-side would reopen
the race. It is now a plain natural-key read because the preceding upsert
already holds the affected row until commit. A different-generated-ID
regression test covers the MySQL/H2 behavior.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -374,4 +370,87 @@ private void
fillTablePOBuilderParentEntityId(TablePO.Builder builder, Namespace
builder.withCatalogId(namespacedEntityId.namespaceIds()[1]);
builder.withSchemaId(namespacedEntityId.entityId());
}
+
+ private TablePO tablePOWithPersistedVersions(TablePO incomingPO, TablePO
persistedPO) {
+ return TablePO.builder()
+ .withTableId(incomingPO.getTableId())
+ .withTableName(incomingPO.getTableName())
+ .withMetalakeId(incomingPO.getMetalakeId())
+ .withCatalogId(incomingPO.getCatalogId())
+ .withSchemaId(incomingPO.getSchemaId())
+ .withAuditInfo(incomingPO.getAuditInfo())
+ .withCurrentVersion(persistedPO.getCurrentVersion())
+ .withLastVersion(persistedPO.getLastVersion())
+ .withDeletedAt(incomingPO.getDeletedAt())
+ .withFormat(incomingPO.getFormat())
+ .withProperties(incomingPO.getProperties())
+ .withPartitions(incomingPO.getPartitions())
+ .withSortOrders(incomingPO.getSortOrders())
+ .withDistribution(incomingPO.getDistribution())
+ .withIndexes(incomingPO.getIndexes())
+ .withComment(incomingPO.getComment())
+ .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:
I agree with the direction, but I would keep that refactor out of this PR.
PR #12577 centralizes the schema-lock transaction entry point; it does not
centralize zero-row CAS classification. Today only schema and table have this
exact contract, while their locking selectors and stable-identity predicates
differ. A shared helper now would mostly move those differences into callbacks.
Once the next entity OCC implementation lands, we can extract the common shape
with at least three concrete callers and shared tests.
--
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]
