jerryshao commented on code in PR #13700:
URL: https://github.com/apache/gravitino/pull/13700#discussion_r4290096661
##########
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:
It's intended, and documented in 4e1da4129. Using the driver's termination
time would have this job reported FAILED (no outcome yet) until the operator
records one. `JobManager` doesn't let a terminal status regress, so that FAILED
would stick even if `Succeeded` came later, and the operator may never record
an outcome once the application is being deleted. A job that finished by itself
but was cancelled in that short window ending CANCELLED is the lesser error.
The Javadoc of `executionInfoOf` now says that "stopped before" means an
outcome or stopped state the operator recorded before the deletion (or whose
time is unknown), and that a driver terminating before the deletion doesn't
count.
##########
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:
Agreed, fixed in 4e1da4129: a stopped state of unknown time counts as before
the deletion, matching `isStopped`. `testMalformedStatus` now marks and deletes
its application and checks that it stays SUCCEEDED, and
`testStoppedStateWithoutTime` covers a stopped state without an outcome.
##########
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:
Fixed in 4e1da4129: if the time of the state the job stopped in is unknown,
the finished time falls back to the deletion time (not before the started
time), as the Javadoc says. Covered by `testStoppedStateWithoutTime` and
`testMalformedStatus`.
--
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]
