jerryshao commented on code in PR #12994:
URL: https://github.com/apache/gravitino/pull/12994#discussion_r3965494647
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/JobMetaService.java:
##########
@@ -103,17 +104,27 @@ public void insertJob(JobEntity jobEntity, boolean
overwrite) throws IOException
JobPO.JobPOBuilder builder = JobPO.builder().withMetalakeId(metalakeId);
JobPO jobPO = JobPO.initializeJobPO(jobEntity, builder);
- SessionUtils.doWithCommit(
- JobMetaMapper.class,
- mapper -> {
- if (overwrite) {
- mapper.insertJobMetaOnDuplicateKeyUpdate(jobPO);
- } else {
- mapper.insertJobMeta(jobPO);
- }
- });
+ long templateId =
+ JobTemplateMetaService.getInstance()
+ .getJobTemplateIdByMetalakeIdAndName(metalakeId,
jobEntity.jobTemplateName());
+ SessionUtils.doMultipleWithCommit(
+ () -> lockMetalake(metalakeName, metalakeId),
+ () ->
+ JobTemplateMetaService.getInstance()
+ .lockTemplateForJobWrite(jobEntity.jobTemplateName(),
templateId, metalakeId),
+ () ->
+ SessionUtils.doWithoutCommit(
+ JobMetaMapper.class,
+ mapper -> {
+ if (overwrite) {
+ mapper.insertJobMetaOnDuplicateKeyUpdate(jobPO);
+ } else {
+ mapper.insertJobMeta(jobPO);
+ }
+ }));
} catch (RuntimeException e) {
ExceptionUtils.checkSQLException(e, Entity.EntityType.JOB,
jobEntity.id().toString());
+ throw e;
Review 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.
##########
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:
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.
--
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]
