jerryshao commented on code in PR #13700:
URL: https://github.com/apache/gravitino/pull/13700#discussion_r4282763981
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationStatusUtils.java:
##########
@@ -180,15 +186,24 @@ public static JobExecutionInfo executionInfoOf(
JobHandle.Status status;
Instant finishedAt = null;
if (K8sJobResourceUtils.isCancelRequested(app)
- && (deletedAt != null || isStopped(current, outcome))) {
+ && deletedAt != null
+ && !stoppedBefore(history, deletedAt)) {
Review Comment:
[Question] This rule reads "had it stopped before the deletion" from the
recorded states only, while the same call has stronger evidence a few lines up:
`driverFinishedAt` from the driver pod (`:176-177`), which `getExecutionInfo`
fetches precisely when there is a deletion timestamp
(`K8sJobExecutor.java:412-418`).
So if the driver exits and the operator hasn't recorded the outcome yet - it
records what it observes, a poll later - a cancel landing in that window marks
and deletes the application (`cancelJob`'s stopped check is state-based too,
`K8sJobExecutor.java:234-236`), and the job is reported CANCELLED with
`finishedAt` set to a driver termination time that precedes the cancel request.
If the operator then writes `Succeeded` with a `lastTransitionTime` after the
deletion timestamp, the outcome is in the status, the driver time says it
finished by itself, and the job still ends up CANCELLED.
Is that the intended line? Where no outcome is ever recorded, CANCELLED is
arguably better than the FAILED that `testDriverTerminationTimeWithoutOutcome`
asserts for the unmarked case, so I'm not proposing
`driverFinishedAt.isBefore(deletedAt)` as a blind extension. But either the
rule should use that evidence, or the Javadoc at `:117-121` should say that
"stopped before" means a recorded state, so the next reader doesn't expect the
driver time to count.
Verified by: reading `SparkApplicationStatusUtils.java:151-196` and
`:314-321`, `K8sJobExecutor.java:230-245` and `:405-420` on fab1005f8b, and the
assertions of
`TestSparkApplicationStatusUtils.testDriverTerminationTimeWithoutOutcome`
(driver terminating *after* the deletion, the mirror case of this one). Not
reproduced against an operator: whether it writes an outcome after the deletion
timestamp I could not check from this repo.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationStatusUtils.java:
##########
@@ -262,6 +307,19 @@ private static boolean isStopped(@Nullable State current,
@Nullable State outcom
|| (!running && outcome != null);
}
+ /**
+ * Returns whether the application had reached an outcome or a stopped state
before it was
+ * deleted. A deletion time is in whole seconds, so a state of the same
second counts as after.
+ */
+ private static boolean stoppedBefore(List<State> history, Instant deletedAt)
{
+ return history.stream()
+ .anyMatch(
+ s ->
+ (isOutcome(s) || STOPPED_STATES.contains(s.summary))
+ && s.time != null
Review Comment:
[Nit] A state whose `lastTransitionTime` is missing or unparsable has `time
== null`, so this predicate doesn't see it and a job that had stopped is
reported CANCELLED instead of keeping its outcome. That precondition is
reachable, and the repo already models it:
`TestSparkApplicationStatusUtils.testMalformedStatus` puts
`"lastTransitionTime": "soon"` on the `Succeeded` state
(`TestSparkApplicationStatusUtils.java:666`) and asserts the resulting
`finishedAt` is null. Add the cancel mark and a deletion to that application
and it reports CANCELLED, although the status says it succeeded.
Note the asymmetry: `isStopped` (`:299-308`), which gates the branch below
and `cancelJob`'s own check, needs no times at all. Treating a stopped state
with an unknown time as "before" (`s.time == null ||
s.time.isBefore(deletedAt)`) would make the two agree, at the cost of not
cancelling a job whose only stopped state has no readable time - which is the
same direction the mapping already takes elsewhere when it can't tell.
Verified by: reading `SparkApplicationStatusUtils.java:314-321`,
`parseState`/`timeOf`/`parseTime` (`:409-434`, `:521-535`, where an unparsable
time becomes null rather than failing) and
`TestSparkApplicationStatusUtils.java:658-672` on fab1005f8b.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationStatusUtils.java:
##########
@@ -163,15 +169,15 @@ public static JobExecutionInfo executionInfoOf(
Instant startedAt = startedAt(history, driverStartedAt, createdAt);
// When the application stopped: when the driver container terminated if
that is known, even
// before the operator records an outcome, else when the application
reached its outcome, else
- // when it was deleted, else when it entered its current state. A deletion
time is in whole
- // seconds, so it may read earlier than a started time of the same second,
which Gravitino
- // would then drop.
+ // when it was deleted if it hadn't stopped by then, else when it entered
its current state. A
+ // deletion time is in whole seconds, so it may read earlier than a
started time of the same
+ // second, which Gravitino would then drop.
Instant stoppedAt;
if (driverFinishedAt != null) {
stoppedAt = driverFinishedAt;
} else if (outcome != null) {
stoppedAt = outcome.driverFinishedAt != null ? outcome.driverFinishedAt
: outcome.time;
- } else if (deletedAt != null) {
+ } else if (deletedAt != null && !isStopped(current, outcome)) {
Review Comment:
[Nit] This guard makes the last sentence of the Javadoc unreachable. It now
reads "finished when its driver container terminated if that is known, else
when it entered its stopped state, or else when it was deleted" (`:141-143`),
but with `isStopped` true the third branch is skipped and the `else` takes
`current.time` with no further fallback: when that time can't be read (the
null-time case of the other comment), `stoppedAt` is null even though
`deletedAt` is known. The job is then reported terminal with no finished time,
and `JobManager.resolveFinishedAt` stamps `Instant.now()` instead
(`core/src/main/java/org/apache/gravitino/job/JobManager.java:1288-1296`), so a
job that stopped minutes ago gets the poll's timestamp.
`stoppedAt = current.time != null ? current.time : deletedAt` in the else
branch would match what the Javadoc promises; otherwise the sentence is worth
trimming.
Verified by: reading `SparkApplicationStatusUtils.java:170-184` (branch
order), `:299-308` (with `outcome == null` here, `isStopped` is true exactly
for `ResourceReleased`/`TerminatedWithoutReleaseResources`) and
`JobManager.java:1282-1296` on fab1005f8b.
--
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]
