[#12992]feat(core): complete OCC for jobs and job templates - #12994
[#12992]feat(core): complete OCC for jobs and job templates#12994yuqi1129 wants to merge 5 commits into
Conversation
| })); | ||
| } catch (RuntimeException e) { | ||
| ExceptionUtils.checkSQLException(e, Entity.EntityType.JOB, jobEntity.id().toString()); | ||
| throw e; |
There was a problem hiding this comment.
insertJob now deliberately throws NoSuchEntityException from the new fencing (lockMetalake / lockTemplateForJobWrite just above), and this throw e correctly stops the old silent swallow. But the caller in JobManager.runJob was not updated, so the newly-escaping exception surfaces as HTTP 500.
(The actual gap is at JobManager.java:498-502, which falls outside this PR's diff — commenting here because this is the line that makes the exception escape.)
try {
entityStore.put(jobEntity, false /* overwrite */);
} catch (IOException e) { // only IOException
throw new RuntimeException("Failed to register the job entity " + jobEntity, e);
}NoSuchEntityException extends RuntimeException, not NotFoundException (api/src/main/java/org/apache/gravitino/exceptions/NoSuchEntityException.java:25), and neither RelationalEntityStore.put nor JDBCBackend.insert converts it. So it reaches JobExceptionHandler (ExceptionHandlers.java:1051-1065), whose instanceof NotFoundException test misses it, falls through to BaseExceptionHandler, and returns Utils.internalError → 500.
Scenario:
- Client A calls
POST .../jobs/runfor templateT.runJobpasses itsgetJobTemplatecheck (JobManager.java:437), creates the staging directory, and submits to the external executor (JobManager.java:473). - Client B deletes template
Tin that window. - A's
entityStore.putreachesinsertJob, andlockTemplateForJobWritethrowsNoSuchEntityException(job_template, T). - A gets an opaque 500 for what
runJob's own signature declares asNoSuchJobTemplateException(404) — while the job is already running on the executor with noJobEntityto poll, cancel, or clean up.
lockMetalake can produce the same escape with NoSuchEntityException(metalake, ...).
Worth noting the inconsistency inside JobManager itself: alterJobTemplate (:343) and cancelJob (:547) both catch NoSuchEntityException and translate it into a proper 404, but runJob and registerJobTemplate do not. On the base commit insertJob had no throw e, so this path was previously swallowed — adding throw e is the right fix, the caller just needs to catch up.
Suggestion: catch NoSuchEntityException around the put in runJob and rethrow it as NoSuchJobTemplateException (logging the now-orphaned jobExecutionId), or add NoSuchEntityException handling to JobExceptionHandler.
There was a problem hiding this comment.
The fix in eb78ed2f7 looks right. runJob now translates this into NoSuchJobTemplateException, which extends NotFoundException, so JobExceptionHandler hits its instanceof NotFoundException branch and returns 404 instead of 500. Thanks also for covering registerJobTemplate in the same pass and for logging the orphaned jobExecutionId — both were exactly the right calls.
One process request: could you reply on each review comment once you have handled it, and resolve the thread? At the moment the code is fixed but both threads are still open with no response, so from the outside it is hard to tell what you consciously addressed versus what is still pending. A one-liner like "fixed in <sha>" or "intentionally leaving this as-is because ..." is plenty.
| + " WHERE job_template_id = #{oldJobTemplateMeta.jobTemplateId}" | ||
| + " AND job_template_name = #{oldJobTemplateMeta.jobTemplateName}" | ||
| + " AND metalake_id = #{oldJobTemplateMeta.metalakeId}" | ||
| + " AND current_version = #{oldJobTemplateMeta.currentVersion}" |
There was a problem hiding this 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:
- Template exists as
{id=X, name="T", v=1}. Thread A callsalterJobTemplate(rename -> "T2");getJobTemplatePOsnapshots{id=X, name="T", v=1}and the updater runs. - Thread B deletes it, then does
put(entity(id=X, name="T3", ...), overwrite=true), leaving{id=X, name="T3", v=1, deleted_at=0}. - A's CAS now matches on
job_template_id = X AND current_version = 1and silently reverts the row to A's old content and name, reporting success. With the old predicates it matched 0 rows and raisedNoSuchEntityException.
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.
There was a problem hiding this comment.
This one still reads unchanged as of eb78ed2f7, which is a perfectly defensible call — I said in the original comment that it is not reachable in production today, since the only entityStore.put for a job template passes overwrite=false.
Could you just confirm here that leaving it is a deliberate decision rather than an oversight, and then resolve the thread? Happy either way; I only want the reasoning on record, since the rest of the OCC series (Function/View) added countDeletedXMetasById guards for the same id-reuse concern.
There was a problem hiding this comment.
Yes, this is intentional. The current job template create path uses overwrite=false, so it cannot reuse a deleted template ID this way. This PR keeps the existing overwrite behavior. If we add overwrite support to this path later, we should also add a check to prevent ID reuse. Thanks for pointing this out.
|
One more nit, on a line that falls outside this PR's diff so it can't be anchored inline — } catch (NoSuchEntityException e) {
throw new NoSuchJobTemplateException(
"Job template with name %s under metalake %s does not exist, this could be due to"
+ " the job template not existing or updated concurrently. For the latter case"
+ " please retry the operation.",
jobTemplateName, metalake);
}That message predates OCC. Now that For contrast, |
yuqi1129
changed the title
Code Coverage Report
Files
|
Changed as suggested. |
Signed-off-by: yuqi <yuqi@datastrato.com>
What changes were proposed in this pull request?
Complete version-CAS updates and soft-deletes for Job and Job Template using the shared OCC helpers. Preserve missing-entity and idempotent-delete behavior while reporting stale writes as optimistic-lock conflicts.
Fence job/template insertion with parent row locks. Delete the template root by expected version, check for nonterminal jobs using a locking read, and cascade by stable template ID in one transaction. If a concurrent insertion commits first, reject deletion and roll back the root change.
Keep background polling and staging cleanup alive after conflicts. Translate disappearing parents into not-found exceptions. After template deletion succeeds, clean only the observed job directories so a same-name replacement's files survive. Retain the template parent directory rather than recursively deleting it.
Reuse metalake fencing and terminal-state checks, and fetch only identity columns for template locking reads.
Why are the changes needed?
Unversioned deletes and unfenced inserts can leave orphan metadata or remove child jobs after a failed template deletion. Checking for active jobs outside the deletion transaction misses concurrent inserts. Removing the entire template-name directory after committing can delete files created by a same-name replacement.
Fix: #12992
Fix: #12993
Does this PR introduce any user-facing change?
Stale writes use the existing OCC conflict contract. Concurrent active jobs prevent template deletion with
InUseException(409). Failed deletes preserve staging files, and successful deletes do not recursively remove a replacement template's directory.No public API signatures, configuration keys, schema, job identifiers, runtime-template JSON, or blind create/import overwrite semantics change. External submission and cancellation are not retried; submission compensation remains tracked in #10271.
How was this patch tested?
TestJobManager,TestJobMetaService,TestJobTemplateMetaService, andTestJobWriteOcc. All relational tests ran against H2, MySQL, and PostgreSQL (-PskipITs -PskipDockerTests=false).TestExceptionHandlerstests passed, including the active-job 409 response../gradlew spotlessApplyandgit diff --checkpassed../gradlew :core:spotlessApply :server:spotlessApply :core:check :server:test --tests org.apache.gravitino.server.web.rest.TestExceptionHandlers :server:check -PskipITs -PskipDockerTests=true -x :core:test.Coverage includes stale writes/deletes, same-name recreation, template rename, parent fencing, cascade rollback, polling/cleanup continuity, cancellation without replay, missing-parent errors, all nonterminal states, and concurrent insertion preventing template deletion. A controlled-interleaving regression reproduced replacement staging-file deletion before the fix.
The full repository suite and external job-executor deployment tests were not run.