jerryshao commented on code in PR #12994:
URL: https://github.com/apache/gravitino/pull/12994#discussion_r3958120981
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/JobTemplateMetaBaseSQLProvider.java:
##########
@@ -137,10 +123,7 @@ public String updateJobTemplateMeta(
+ " last_version = #{newJobTemplateMeta.lastVersion},"
+ " deleted_at = #{newJobTemplateMeta.deletedAt}"
+ " WHERE job_template_id = #{oldJobTemplateMeta.jobTemplateId}"
- + " AND job_template_name = #{oldJobTemplateMeta.jobTemplateName}"
- + " AND metalake_id = #{oldJobTemplateMeta.metalakeId}"
+ " AND current_version = #{oldJobTemplateMeta.currentVersion}"
Review Comment:
Nit / defence-in-depth — **not reachable today**, flagging only because a
guard went away with no replacement.
Dropping `last_version` is clearly fine: `JobTemplatePO.updateJobTemplatePO`
always sets `currentVersion == lastVersion`, so that predicate was redundant
with the retained `current_version` check.
The `job_template_name` / `metalake_id` predicates were doing slightly more,
though. `insertJobTemplateMetaOnDuplicateKeyUpdate` resurrects a soft-deleted
row **by the same `job_template_id`** (`ON DUPLICATE KEY UPDATE`, and `ON
CONFLICT(job_template_id)` on PostgreSQL), resetting `deleted_at = 0` and
`current_version` back to `1`, and it can change `job_template_name` at the
same time. So version numbers are not monotonic across a resurrection, and a
stale snapshot's CAS can match a *different logical entity* that happens to
reuse the id:
1. Template exists as `{id=X, name="T", v=1}`. Thread A calls
`alterJobTemplate(rename -> "T2")`; `getJobTemplatePO` snapshots `{id=X,
name="T", v=1}` and the updater runs.
2. Thread B deletes it, then does `put(entity(id=X, name="T3", ...),
overwrite=true)`, leaving `{id=X, name="T3", v=1, deleted_at=0}`.
3. A's CAS now matches on `job_template_id = X AND current_version = 1` and
silently reverts the row to A's old content and name, reporting success. With
the old predicates it matched 0 rows and raised `NoSuchEntityException`.
Note that `writeFailure`'s name/`metalake_id` predicate does not cover this,
since it only runs when the CAS matches **zero** rows.
I checked the reachability and it is fine today: the only production
`entityStore.put` for a job template passes `overwrite=false`
(`JobManager.java:224`), so a same-id resurrection cannot happen through the
REST path. So this is not a bug in the current code — just worth a conscious
decision, especially since the Function/View PRs in this same OCC series added
`countDeletedXMetasById` guards precisely to reject reusing a soft-deleted ID.
--
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]
