jerryshao commented on code in PR #12994:
URL: https://github.com/apache/gravitino/pull/12994#discussion_r3958102173
##########
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:
`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.)_
```java
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:
1. Client A calls `POST .../jobs/run` for template `T`. `runJob` passes its
`getJobTemplate` check (`JobManager.java:437`), creates the staging directory,
and submits to the external executor (`JobManager.java:473`).
2. Client B deletes template `T` in that window.
3. A's `entityStore.put` reaches `insertJob`, and `lockTemplateForJobWrite`
throws `NoSuchEntityException(job_template, T)`.
4. A gets an opaque 500 for what `runJob`'s own signature declares as
`NoSuchJobTemplateException` (404) — while the job is already running on the
executor with no `JobEntity` to 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`.
--
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]
