jerryshao commented on code in PR #12649:
URL: https://github.com/apache/gravitino/pull/12649#discussion_r3891653506
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelMetaService.java:
##########
@@ -206,10 +155,58 @@ public boolean deleteModel(NameIdentifier ident) {
Entity.EntityType.MODEL.name(),
modelFullName,
OperateType.DROP));
- }
- });
+ });
+ } catch (NoSuchEntityException e) {
+ // Another writer dropped the model between the read above and this
transaction. A drop that
+ // finds nothing to drop is reported the same way as the read above
reports it, so that a
+ // duplicate drop stays a plain "false" instead of surfacing as an error.
+ LOG.warn("Failed to delete model: {}", ident, e);
+ return false;
+ }
- return modelDeletedCount.get() > 0;
+ return true;
+ }
+
+ /**
+ * Deletes a model row only when its concurrency version has not changed
since it was read.
+ *
+ * <p>The caller must run this method in the same transaction that removes
the model's related
+ * data. This allows any later cleanup failure to restore the model row as
well.
+ */
+ void deleteModelWithVersion(NameIdentifier ident, ModelPO observedModelPO) {
Review Comment:
**conventions**: New package-private methods `deleteModelWithVersion` (line
176) and `bumpModelVersion` (line 198) are inserted between two public methods
(`deleteModel` at 128, `deleteModelMetasByLegacyTimeline` at 212), breaking the
visibility grouping CLAUDE.md requires.
The repo-root CLAUDE.md states: "Class Member Ordering: Follow the order:
... 5. Methods (Group by visibility, putting `private` methods at the end)."
Before this PR, `deleteModel` and `deleteModelMetasByLegacyTimeline` were
adjacent public methods. This diff splices two package-private methods between
them, so the sequence becomes public -> package-private -> package-private ->
public instead of all public methods being grouped together.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java:
##########
@@ -462,4 +483,30 @@ private NoSuchEntityException
noSuchModelException(NameIdentifier modelIdentifie
Entity.EntityType.MODEL.name().toLowerCase(Locale.ROOT),
modelIdentifier.toString());
}
+
+ /**
+ * Decides which error a model-version write that matched no row should
report.
+ *
+ * <p>The write compares the values it read, so it matches nothing either
because the version was
+ * deleted, which is a missing entity, or because somebody else changed the
same version first,
+ * which is a conflict.
+ */
+ private RuntimeException modelVersionWriteFailure(
+ NameIdentifier ident, Long modelId, ModelVersionPO observedVersionPO) {
+ List<ModelVersionPO> currentVersionPOs =
+ SessionUtils.getWithoutCommit(
+ ModelVersionMetaMapper.class,
+ mapper -> mapper.selectModelVersionMeta(modelId,
observedVersionPO.getModelVersion()));
+ if (currentVersionPOs.isEmpty()) {
+ return noSuchModelVersionException(ident);
Review Comment:
**test-coverage**: The `NoSuchEntityException` branch of
`modelVersionWriteFailure` (taken when the version row was concurrently deleted
during `updateModelVersion`) is never exercised by a test — only the
concurrent-modification branch is tested.
CLAUDE.md: "NO tests = NO merge."
`testModelVersionAlterRejectsStaleAggregateVersion` only drives a concurrent
update of the version row, never a concurrent delete between the read and the
update, so the `isEmpty()` -> `noSuchModelVersionException()` path in
`modelVersionWriteFailure` (lines 494-503) has no coverage; a bug that
mis-classifies a deleted-version conflict as a plain optimistic-lock conflict
(or vice versa) would go undetected.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java:
##########
@@ -214,52 +220,68 @@ public boolean deleteModelVersion(NameIdentifier ident) {
NameIdentifierUtil.checkModelVersion(ident);
NameIdentifier modelIdent = NameIdentifier.of(ident.namespace().levels());
- // Will throw a NoSuchEntityException if the model does not exist.
- ModelEntity modelEntity;
+ ModelPO modelPO;
try {
- modelEntity =
ModelMetaService.getInstance().getModelByIdentifier(modelIdent);
+ modelPO =
ModelMetaService.getInstance().getModelPOByIdentifier(modelIdent);
} catch (NoSuchEntityException e) {
return false;
}
boolean isVersionNumber = NumberUtils.isCreatable(ident.name());
+ // Resolve an alias to its numeric model-version value once. The
concurrency-version check
+ // below will fail if another writer changes the version or alias after
this read.
+ List<ModelVersionPO> observedVersionPOs =
Review Comment:
**correctness (high)**: `deleteModelVersion` resolves an alias to a numeric
version via an unlocked pre-transaction `SELECT`, then deletes by that stale
captured version inside a transaction guarded only by the identity-only
`bumpModelVersion` check — so a concurrent alias reassignment causes it to
silently delete the wrong version.
Failure scenario: Model version 3 is aliased "prod". Client A calls
`deleteModelVersion(ident="prod")`; the read here resolves `modelVersion=3`
before any transaction starts. Before A's transaction runs, client B calls
`updateModelVersion` to move alias "prod" from version 3 to version 5. A's
transaction then executes: `bumpModelVersion` (line 255) succeeds because it
only checks model identity (model_id/schema_id/model_name), not the resolved
version or alias state; A proceeds to
`softDeleteModelVersionMetaByModelIdAndVersion(modelId, 3)` (line 263-264) and
deletes version 3's aliases (line 274). The call returns `true`, but it deleted
version 3 (no longer aliased "prod") while the actual "prod" (now version 5) is
untouched — silent data loss with no error surfaced. The old code this replaced
resolved the alias live inside the same guarded UPDATE
(`softDeleteModelVersionMetaByModelIdAndAlias`), which prevented this; no new
test in this diff covers alias-reassignment races (only
model-drop races are tested).
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java:
##########
@@ -356,61 +378,64 @@ public <E extends Entity & HasIdentifier>
ModelVersionEntity updateModelVersion(
boolean isModelVersionUriUpdated =
isModelVersionUriUpdated(oldModelVersionEntity, newModelVersionEntity);
- final AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- // URI and alias updates can reinsert active model-version rows, so
they need the same
- // schema fence as a new version registration.
+ // Keep the schema locked because URI and alias changes may replace
active child rows.
() -> lockSchemaForModelVersionWrite(modelIdent, modelPO),
+ // Advance the shared concurrency version before changing child
rows. A competing model
+ // or model-version writer then makes this operation fail without
leaving partial changes.
+ () -> ModelMetaService.getInstance().bumpModelVersion(modelIdent,
modelPO),
Review Comment:
**correctness (high)**: `updateModelVersion` has the identical
stale-alias-resolution race as `deleteModelVersion`: the alias is resolved to a
numeric version via an unlocked pre-transaction read, then the URI/alias
mutation is applied using that stale captured version, guarded only by
identity-only `bumpModelVersion`.
Failure scenario: Same setup: "prod" is reassigned from version 3 to version
5 between the read at line 332-341 (`oldModelVersionPOs`) and the transaction.
A caller invokes `updateModelVersion(ident="prod", updater)` intending to edit
the version currently named "prod". Because version 3's row still exists, the
CAS at line 416 (`updated != 0`) succeeds — `bumpModelVersion` at line 387
never detects the alias moved — so the update silently overwrites version 3's
URI (line 393-404) or fields (line 407-414) and clobbers version 3's alias rows
(line 429-437), even though the caller meant version 5. The call returns the
new entity successfully with no indication it operated on the wrong version. No
test in this diff (`testModelVersionAlterRejectsStaleAggregateVersion`, etc.)
exercises an alias reassigned to a different version between the read and the
write.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java:
##########
@@ -214,52 +220,68 @@ public boolean deleteModelVersion(NameIdentifier ident) {
NameIdentifierUtil.checkModelVersion(ident);
NameIdentifier modelIdent = NameIdentifier.of(ident.namespace().levels());
- // Will throw a NoSuchEntityException if the model does not exist.
- ModelEntity modelEntity;
+ ModelPO modelPO;
try {
- modelEntity =
ModelMetaService.getInstance().getModelByIdentifier(modelIdent);
+ modelPO =
ModelMetaService.getInstance().getModelPOByIdentifier(modelIdent);
} catch (NoSuchEntityException e) {
return false;
}
boolean isVersionNumber = NumberUtils.isCreatable(ident.name());
+ // Resolve an alias to its numeric model-version value once. The
concurrency-version check
+ // below will fail if another writer changes the version or alias after
this read.
+ List<ModelVersionPO> observedVersionPOs =
+ SessionUtils.getWithoutCommit(
+ ModelVersionMetaMapper.class,
+ mapper -> {
+ if (isVersionNumber) {
+ return mapper.selectModelVersionMeta(
+ modelPO.getModelId(), Integer.valueOf(ident.name()));
+ }
+ return
mapper.selectModelVersionMetaByAlias(modelPO.getModelId(), ident.name());
+ });
+ if (observedVersionPOs.isEmpty()) {
+ return false;
+ }
+ Integer modelVersion = observedVersionPOs.get(0).getModelVersion();
- AtomicInteger modelVersionDeletedCount = new AtomicInteger();
- SessionUtils.doMultipleWithCommit(
- // Delete model version relations first
- () ->
- modelVersionDeletedCount.set(
+ try {
+ SessionUtils.doMultipleWithCommit(
+ // Keep the parent schema from being deleted while this transaction
changes version rows.
+ () -> lockSchemaForModelVersionWrite(modelIdent, modelPO),
+ // Reserve this write by advancing the shared concurrency version
before deleting
+ // anything. If the model changed after the read above, leave the
version and its aliases
+ // untouched.
+ () -> ModelMetaService.getInstance().bumpModelVersion(modelIdent,
modelPO),
+ () -> {
+ // An alias was resolved to its numeric version above. Delete
every URI row belonging to
+ // that version, regardless of whether the caller supplied the
number or an alias.
+ int deleted =
SessionUtils.getWithoutCommit(
ModelVersionMetaMapper.class,
- mapper -> {
- if (isVersionNumber) {
- return
mapper.softDeleteModelVersionMetaByModelIdAndVersion(
- modelEntity.id(), Integer.valueOf(ident.name()));
- } else {
- return
mapper.softDeleteModelVersionMetaByModelIdAndAlias(
- modelEntity.id(), ident.name());
- }
- })),
- () -> {
- // Delete model version alias relations
- if (modelVersionDeletedCount.get() == 0) {
- return;
- }
-
- SessionUtils.doWithoutCommit(
- ModelVersionAliasRelMapper.class,
- mapper -> {
- if (isVersionNumber) {
- mapper.softDeleteModelVersionAliasRelsByModelIdAndVersion(
- modelEntity.id(), Integer.valueOf(ident.name()));
- } else {
- mapper.softDeleteModelVersionAliasRelsByModelIdAndAlias(
- modelEntity.id(), ident.name());
- }
- });
- });
-
- return modelVersionDeletedCount.get() > 0;
+ mapper ->
+ mapper.softDeleteModelVersionMetaByModelIdAndVersion(
+ modelPO.getModelId(), modelVersion));
+ if (deleted == 0) {
+ throw noSuchModelVersionException(ident);
Review Comment:
**test-coverage**: The new version-specific CAS-failure branch in
`deleteModelVersion` (thrown when
`softDeleteModelVersionMetaByModelIdAndVersion` matches zero rows even though
`bumpModelVersion` already succeeded) has no dedicated test.
CLAUDE.md states "Write unit tests for ALL new logic. NO tests = NO merge."
This branch fires when the specific version row is concurrently deleted while
the parent model still exists. The only related test,
`testDeleteModelVersionLosingRaceToModelDropReturnsFalse`, drops the whole
model instead, exercising `bumpModelVersion`'s failure path in
`ModelMetaService`, never reaching this branch — so a regression here (e.g.
losing the `noSuchModelVersionException` distinction) would not be caught by
the added suite.
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java:
##########
@@ -1708,14 +1711,18 @@ public static ModelVersionEntity fromModelVersionPO(
}
/**
- * Updata ModelPO with new ModelEntity object, metalakeID, catalogID,
schemaID will be the same as
- * the old one. the id, name, comment, properties, latestVersion and
auditInfo will be updated.
+ * Creates a {@link ModelPO} for a model update.
+ *
+ * <p>The parent metalake, catalog, and schema IDs stay unchanged. The model
fields come from the
+ * new entity, and the shared concurrency version advances by one.
*
- * @param oldModelPO the old ModelPO object
- * @param newModel the new ModelEntity object
- * @return the updated ModelPO object
+ * @param oldModelPO the model record read before the update
+ * @param newModel the updated model entity
+ * @return the model record to store
*/
public static ModelPO updateModelPO(ModelPO oldModelPO, ModelEntity
newModel) {
+ long nextModelVersion =
+ Math.max(oldModelPO.getCurrentVersion(), oldModelPO.getLastVersion())
+ 1;
Review Comment:
**simplification**: `updateModelPO` computes the next version as
`Math.max(currentVersion, lastVersion) + 1`, even though every write path for
models always sets `current_version` and `last_version` to the identical value
— every other entity (Metalake, Catalog, Schema, Table) in this same file just
does `lastVersion + 1`.
Metalake/Catalog/Schema use `oldPO.getCurrentVersion() + 1` and Table uses
`oldPO.getLastVersion() + 1` directly (no `Math.max`) at
POConverters.java:144/243/341/461, while `bumpModelVersion` and
`insertModelMetaOnDuplicateKeyUpdate` always set `current_version` and
`last_version` to the same value for models too. A future maintainer reading
`Math.max(current, last)` will assume the two columns can legitimately diverge
for models and waste time investigating a divergence path that doesn't exist,
or 'fix' this into a real bug by making the fields track different things.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelMetaService.java:
##########
@@ -447,4 +446,61 @@ public List<ModelEntity>
batchGetModelByIdentifier(List<NameIdentifier> identifi
return POConverters.fromModelPOs(modelPOs, firstIdent.namespace());
});
}
+
+ /**
+ * Explains why a version-checked model write changed no rows.
+ *
+ * <p>The locking read waits for a competing transaction to finish. If the
same model is still at
+ * the requested name, only its concurrency version changed and the caller
should retry. If it was
+ * deleted, renamed, or moved, the requested model no longer exists.
+ */
+ RuntimeException modelWriteFailure(NameIdentifier ident, ModelPO
observedModelPO) {
Review Comment:
**reuse**: `modelWriteFailure` reimplements, field-for-field, the same "lock
row, classify not-found vs. concurrent-modification" pattern already present as
`tableWriteFailure`, `schemaWriteFailure`, `catalogWriteFailure`, and
`metalakeWriteFailure`, instead of sharing a common helper.
All five methods do: locking select-for-update by stable ID ->
identity-mismatch check -> `NoSuchEntityException` or
`ExceptionUtils.concurrentModification`. This PR adds a sixth (plus
`modelVersionWriteFailure`, a seventh) near-identical copy. A future fix to
this classification logic (e.g. adding a "renamed" vs "deleted" distinction)
must be hand-applied across 6-7 copies, and the copies are already inconsistent
(Model checks four identity fields; ModelVersion checks only existence).
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java:
##########
@@ -173,10 +176,13 @@ public void insertModelVersion(ModelVersionEntity
modelVersionEntity) throws IOE
try {
SessionUtils.doMultipleWithCommit(
- // Model versions carry the schema ID directly, so they must take
the same parent fence
- // as models. Otherwise a schema cascade can pass its model-version
cleanup and a
- // concurrent registration can insert a new active version below the
deleted schema.
+ // Keep the schema locked while adding the version. Otherwise a
concurrent schema delete
+ // could finish its cleanup just before these new rows are inserted.
() -> lockSchemaForModelVersionWrite(modelIdent, modelPO),
+ // Advance the concurrency version shared by the model and all its
versions before
+ // inserting URI or alias rows. A competing writer makes this check
fail before any
+ // partial data is added.
+ () -> ModelMetaService.getInstance().bumpModelVersion(modelIdent,
modelPO),
Review Comment:
**simplification**: The two-step preamble
`lockSchemaForModelVersionWrite(...)` then
`ModelMetaService.getInstance().bumpModelVersion(...)` is copy-pasted verbatim
as the first two lambdas of `doMultipleWithCommit` in `insertModelVersion`
(181-185), `deleteModelVersion` (251-255), and `updateModelVersion` (384-387).
If the locking/version-bump protocol ever needs to change (e.g. reordering,
adding a check, or logging), a maintainer must find and update three separate
lambda pairs; missing one silently reintroduces the race this PR is meant to
close. A shared private helper, e.g. `reserveModelVersionWrite(modelIdent,
modelPO)`, invoked as a single lambda at each call site, would remove the risk
of the three drifting out of sync.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java:
##########
@@ -190,9 +196,9 @@ public void insertModelVersion(ModelVersionEntity
modelVersionEntity) throws IOE
mapper -> mapper.insertModelVersionAliasRels(aliasRelPOs));
},
() -> {
- // If the model version is inserted successfully, update the model
latest version. A
- // zero result means the model disappeared after the read above,
so the inserted version
- // and aliases must roll back with this transaction.
+ // The insert statements above use the old model_latest_version as
the number of the new
+ // version. Increment it only after those inserts. The earlier
version check keeps the
+ // model row locked, so another registration cannot choose the
same number.
int updated =
Review Comment:
**efficiency**: `insertModelVersion` issues two separate UPDATE round trips
against the same `model_meta` row in sequence — `bumpModelVersion` (line 185)
and `updateModelLatestVersion` (line 202-204) — that could be merged into a
single UPDATE setting all three columns.
Every model-version registration costs 2 UPDATE round trips on the model row
instead of 1, both keyed on the same `WHERE model_id=?` (plus
schema_id/model_name for the first). Merging
`current_version`/`last_version`/`model_latest_version` into one UPDATE with
the same WHERE clause would halve DB round trips on this hot write path and
reduce lock hold time on the model row under concurrent version registrations.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/ModelMetaBaseSQLProvider.java:
##########
@@ -251,6 +290,29 @@ public String updateModelLatestVersion(@Param("modelId")
Long modelId) {
+ " WHERE model_id = #{modelId} AND deleted_at = 0";
}
+ /**
+ * Returns SQL that advances the shared model concurrency version while the
model still exists
+ * under the name the caller resolved.
+ *
+ * @param modelId the model ID
+ * @param schemaId the ID of the schema holding the model
+ * @param modelName the model name the caller resolved
+ * @return the version bump SQL
+ */
+ public String bumpModelVersion(
Review Comment:
**altitude**: Three different hand-written SQL guard strategies coexist for
the model row: a true CAS on `current_version`
(`softDeleteModelMetaByIdAndVersion`, `updateModelMeta`) and an identity-only
fence with no version comparison at all (`bumpModelVersion`), with no shared
builder for "generate a WHERE clause that CAS-checks a version column."
Each method's WHERE clause was typed by hand and already differs (one checks
`current_version=`, one checks only identity + `deleted_at=0`). The next entity
wanting this OCC pattern, or a future column added to the guard, requires
copying and re-verifying one of these variants by hand rather than calling one
parametrized helper — a plausible source of an under-guarded CAS clause that
silently reintroduces the exact race this PR is fixing.
--
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]
