yuqi1129 commented on code in PR #12509:
URL: https://github.com/apache/gravitino/pull/12509#discussion_r3811789575
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -592,11 +595,30 @@ void pullAndUpdateJobStatus() {
}
if (newStatus != job.status()) {
+ boolean isStarted = newStatus == JobHandle.Status.STARTED;
boolean isFinished =
newStatus == JobHandle.Status.SUCCEEDED
|| newStatus == JobHandle.Status.FAILED
|| newStatus == JobHandle.Status.CANCELLED;
+ // SUCCEEDED/FAILED prove the job actually ran, even when no
poll ever observed
+ // it as STARTED (e.g. it transitioned QUEUED -> terminal
between two polls). In
+ // that case, fall back to the job's queued time as the
best-known lower bound for
+ // startedAt - leaving it unset would incorrectly imply the job
never started.
+ // CANCELLED is deliberately excluded: a cancelled job may have
been killed while
+ // still QUEUED and never started at all, so leaving startedAt
unset stays
+ // accurate there.
+ boolean provenToHaveStarted =
Review Comment:
Can we avoid inferring `startedAt` from the terminal status? `FAILED` does
not prove that execution started: this method converts `NoSuchJobException` to
`FAILED` above, and `LocalJobExecutor` also records `FAILED` when process
creation/start throws before it records `STARTED`. Even `SUCCEEDED` only proves
that the job ran, not when it started; `auditInfo().createTime()` is the queued
time. Returning that value as `startedAt` reports zero queue latency and
inflates execution time. Since `startedAt` is nullable, it should remain
unknown when no `STARTED` transition was observed, or `JobExecutor` should
expose the actual start timestamp.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -517,7 +518,9 @@ public JobEntity cancelJob(String metalake, String jobId)
throws NoSuchJobExcept
.withLastModifier(PrincipalUtils.getCurrentPrincipal().getName())
.withLastModifiedTime(Instant.now())
.build())
- // CANCELLING is not a terminal state, so the job is not finished
yet.
+ // CANCELLING is not a terminal state; carry forward whatever
startedAt/finishedAt
+ // the job already had.
+ .withStartedAt(jobEntity.startedAt())
Review Comment:
This copies `startedAt` from the snapshot fetched before the external
cancellation call and before acquiring the write lock. A concurrent status poll
can persist `STARTED` with a nonzero `startedAt` during that gap, after which
this overwrite stores `CANCELLING` with the stale zero value. Please merge the
transition against the latest entity under the lock/transaction so a recorded
start time cannot be lost.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -592,11 +595,30 @@ void pullAndUpdateJobStatus() {
}
if (newStatus != job.status()) {
+ boolean isStarted = newStatus == JobHandle.Status.STARTED;
boolean isFinished =
newStatus == JobHandle.Status.SUCCEEDED
|| newStatus == JobHandle.Status.FAILED
|| newStatus == JobHandle.Status.CANCELLED;
+ // SUCCEEDED/FAILED prove the job actually ran, even when no
poll ever observed
+ // it as STARTED (e.g. it transitioned QUEUED -> terminal
between two polls). In
+ // that case, fall back to the job's queued time as the
best-known lower bound for
+ // startedAt - leaving it unset would incorrectly imply the job
never started.
+ // CANCELLED is deliberately excluded: a cancelled job may have
been killed while
+ // still QUEUED and never started at all, so leaving startedAt
unset stays
+ // accurate there.
+ boolean provenToHaveStarted =
+ newStatus == JobHandle.Status.SUCCEEDED || newStatus ==
JobHandle.Status.FAILED;
+ long startedAt;
+ if (isStarted) {
+ startedAt = Instant.now().toEpochMilli();
Review Comment:
Please preserve an existing positive `job.startedAt()` here. `cancelJob`
carries the start time into `CANCELLING`, and cancellation is asynchronous, so
an executor can still report `STARTED` while cancellation is in flight. A
`CANCELLING -> STARTED` observation would enter this branch and replace the
previously recorded start time with a later poll time.
--
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]
