jerryshao commented on code in PR #12497:
URL: https://github.com/apache/gravitino/pull/12497#discussion_r3809285980
##########
api/src/main/java/org/apache/gravitino/job/JobHandle.java:
##########
@@ -69,4 +71,13 @@ enum Status {
* @return the status of the job
*/
Status jobStatus();
+
+ /**
+ * Get the time when the job finished execution.
+ *
+ * @return the finished time of the job, or null if the job has not finished
execution yet
+ */
+ default Instant finishedAt() {
+ throw new UnsupportedOperationException("finishedAt is not implemented");
Review Comment:
Done, the default exception message now includes the implementing class name
and instructs overriding the method, in 4f85fd94d.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -588,6 +592,11 @@ void pullAndUpdateJobStatus() {
}
if (newStatus != job.status()) {
+ boolean isFinished =
+ newStatus == JobHandle.Status.SUCCEEDED
+ || newStatus == JobHandle.Status.FAILED
+ || newStatus == JobHandle.Status.CANCELLED;
Review Comment:
This scenario isn't actually reachable today: `activeJobs` (filtered a few
lines above) only includes jobs whose status is QUEUED/STARTED/CANCELLING, and
every JobEntity construction site in this class carries forward or explicitly
sets finishedAt=0 for non-terminal statuses. So by the time a job reaches this
branch with newStatus terminal, its prior finishedAt is guaranteed to be 0 —
there's no terminal-with-a-real-timestamp job that can re-enter this code path.
Adding a `job.finishedAt() > 0 ? ... : Instant.now()` guard would be dead code
today rather than a real safeguard, so leaving as-is per this repo's guidance
against validating unreachable states. Thanks for flagging the coupling though
— it's a fair point that this correctness depends on the activeJobs filter
staying as-is.
--
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]
