lasdf1234 commented on code in PR #14047:
URL: https://github.com/apache/gravitino/pull/14047#discussion_r4334631314
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -110,6 +110,14 @@ public class JobManager implements JobOperationDispatcher {
private static final long JOB_STATUS_PULL_MIN_INTERVAL_IN_MS = 60 * 1000L;
// 1 minute
+ // A live owner of a node local job cancels it at its next status pull, and
records it as
+ // CANCELLED at the pull after the process exits, so it answers a
cancellation within about two
+ // pull intervals, as long as the process exits within one, e.g. it is
killed forcibly within
+ // gravitino.jobExecutor.local.cancelForceKillDelayInMs. A job still
CANCELLING after this many
+ // pull intervals is considered left behind by an owner that has exited.
This assumes all servers
+ // use the same pull interval and have their clocks in sync.
+ private static final int CANCELLATION_TIMEOUT_IN_PULL_INTERVALS = 3;
Review Comment:
**[Medium] Timeout is `3 × pullInterval` only — false `CANCELLED` is easy
with the documented local tuning**
The comment says a live owner answers within ~two pull intervals *as long as
the process exits within one* (e.g. within `cancelForceKillDelayInMs`).
Worst-case owner timeline is closer to:
1. up to **1× interval** before the owner sees `CANCELLING` and calls cancel,
2. up to **`cancelForceKillDelayInMs`** (default **30s**) before SIGKILL,
3. up to **1× interval** before the owner records `CANCELLED`.
So wall time can exceed `3 × interval` whenever `cancelForceKillDelayInMs ≥
statusPullIntervalInMs`.
That is exactly the combo the docs push for local/dev work
(`statusPullIntervalInMs=10000` + default force-kill `30000` → ~50s worst case
vs **30s** settle timeout). Production defaults (5 min interval) are fine; the
hazard is the recommended short interval.
Docs already list this under the caution — good — but this comment reads
safer than the math. Prefer either:
- bake force-kill into the timeout, e.g. roughly `2 × interval +
cancelForceKillDelay + slack`, or
- tighten the comment to state the assumption `cancelForceKillDelayInMs <
statusPullIntervalInMs` (and that the 10s local tuning violates it unless
force-kill is also lowered).
Once metadata is `CANCELLED`, a still-alive owner never asks the process to
stop (also documented).
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -1031,17 +1043,21 @@ private void pullAndUpdateJobStatus(String metalake) {
|| job.status() == JobHandle.Status.CANCELLING)
.toList();
+ long now = System.currentTimeMillis();
activeJobs.forEach(
job -> {
// A failure on one job must never stop the status pull of the
others.
try {
// Only the job executor instance owning the job can query its
status. The jobs
- // owned by other servers are skipped, and the jobs left behind by
a server that has
- // exited are settled by cleanUpStagingDirs() once they expire.
- if (!jobExecutor.ownsJob(job.jobExecutionId())) {
- return;
+ // owned by other servers are skipped, except a node local job
whose cancellation its
+ // owner has not answered in time: the owner has likely exited,
e.g. the server
+ // restarted, so nobody else would ever settle it. The other jobs
left behind by a
+ // server that has exited are settled by cleanUpStagingDirs() once
they expire.
+ if (jobExecutor.ownsJob(job.jobExecutionId())) {
+ pullAndUpdateOwnedJobStatus(metalake, job);
+ } else if (jobExecutor.isJobStateNodeLocal() &&
isCancellationUnanswered(job, now)) {
+ settleUnansweredCancellation(metalake, job, now);
Review Comment:
**[Low / test gap] Settle path lacks the same failure-isolation coverage as
the owned-job path**
Happy path, within-timeout wait, non-node-local skip, and concurrent/OCC
answer are covered. Missing parallels to existing pull tests:
- `settleUnansweredCancellation` / `updateJobEntity` throwing on one job
still lets other jobs in the same pull proceed (cf.
`testPullJobStatusContinuesAfterFailingToUpdateAJob`)
- `isJobStateNodeLocal()` throwing on this branch (cf.
`testPullJobStatusContinuesAfterFailingToCheckJobOwner`)
Not blocking if you rely on the outer per-job `catch (Throwable)`; a couple
of tests would lock the intent in.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -1443,6 +1459,39 @@ private boolean isStaleActiveJob(JobEntity job, long
now) {
&& lastUpdatedTimeInMs(job) + jobStagingDirKeepTimeInMs < now;
}
+ private void settleUnansweredCancellation(String metalake, JobEntity job,
long now) {
+ AtomicBoolean settled = new AtomicBoolean(false);
+ updateJobEntity(
+ metalake,
+ job,
+ latestJobEntity -> {
+ // The owner may have answered the cancellation since the
listJobs() snapshot.
+ if (!isCancellationUnanswered(latestJobEntity, now)) {
+ return latestJobEntity;
+ }
+ settled.set(true);
+ return toUpdatedStatusJobEntity(
+ latestJobEntity,
JobExecutionInfo.of(JobHandle.Status.CANCELLED));
+ })
+ .filter(settledJob -> settled.get())
+ .ifPresent(
+ settledJob ->
+ LOG.warn(
+ "Job {} with execution id {} under metalake {} has been
cancelling for more "
+ + "than {} ms without its owner answering, marking it
as {}. This could be "
+ + "due to the Gravitino server running the job having
exited.",
+ job.name(),
+ job.jobExecutionId(),
+ metalake,
+ cancellationTimeoutInMs,
+ settledJob.status()));
+ }
+
+ private boolean isCancellationUnanswered(JobEntity job, long now) {
+ return job.status() == JobHandle.Status.CANCELLING
+ && now - lastUpdatedTimeInMs(job) > cancellationTimeoutInMs;
Review Comment:
**[Low] Strict `>` + poll alignment → settle can wait ~4× interval**
`now - lastUpdated > cancellationTimeoutInMs` means exactly `3 × interval`
is still "answered". Combined with settlement only on poll ticks, wall-clock
clear time is often closer to **four** intervals (docs already say "about four
times" — good).
Optional: add a unit test at the exact boundary (`== 3×interval` stays
`CANCELLING`, `3×interval + 1ms` settles) so the `>` vs `>=` choice can't
regress silently.
--
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]
