yuqi1129 commented on code in PR #12675:
URL: https://github.com/apache/gravitino/pull/12675#discussion_r3878872945
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -545,6 +532,27 @@ public JobEntity cancelJob(String metalake, String jobId)
throws NoSuchJobExcept
});
}
+ private JobEntity toCancellingJobEntity(JobEntity latestJobEntity) {
+ return JobEntity.builder()
+ .withId(latestJobEntity.id())
+ .withJobExecutionId(latestJobEntity.jobExecutionId())
+ .withJobTemplateName(latestJobEntity.jobTemplateName())
+ .withStatus(JobHandle.Status.CANCELLING)
Review Comment:
Blocking: this updater still unconditionally changes the latest entity to
`CANCELLING`. There is a race because the external cancel call happens before
this locked update: the initial snapshot can be active, then a concurrent
status poll can persist `SUCCEEDED`, `FAILED`, or `CANCELLED` while
cancellation is in flight. `entityStore.update()` will re-fetch that terminal
entity here, but this line regresses it back to `CANCELLING`. Please preserve
the latest entity when it is already terminal (and when it is already
`CANCELLING`), similar to `toUpdatedStatusJobEntity`, and add a test where the
initial snapshot is active but the updater receives a terminal entity.
--
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]
