Copilot commented on code in PR #12497:
URL: https://github.com/apache/gravitino/pull/12497#discussion_r3804773562
##########
docs/open-api/jobs.yaml:
##########
@@ -514,6 +514,10 @@ components:
- "canceled"
audit:
$ref: "./openapi.yaml#/components/schemas/Audit"
+ finishedAt:
+ type: string
+ format: date-time
+ description: The time when the job finished execution, or absent if
the job has not finished execution yet
Review Comment:
The schema declares `finishedAt` as a non-null `string`, but server/client
tests added in this PR assert that unfinished jobs round-trip `finishedAt` as
`null` over the wire. If the API can return `\"finishedAt\": null`, the OpenAPI
schema should explicitly allow it (e.g., `nullable: true` or a `oneOf`
including `null`). Alternatively, ensure serialization omits the field entirely
when unfinished and align the description/examples accordingly.
##########
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:
The default exception message is fairly generic for external implementers.
Consider making it more actionable (e.g., explicitly instructing implementers
to override `finishedAt()` or indicating the interface/class name) to reduce
debugging time when third-party implementations hit this at runtime.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -602,6 +611,7 @@ void pullAndUpdateJobStatus() {
.withLastModifier(PrincipalUtils.getCurrentPrincipal().getName())
.withLastModifiedTime(Instant.now())
.build())
+ .withFinishedAt(isFinished ?
Instant.now().toEpochMilli() : job.finishedAt())
Review Comment:
`finishedAt` is overwritten with `Instant.now()` on *any* transition where
`newStatus` is terminal, even if the job already had a positive `finishedAt`
(e.g., a terminal→terminal transition due to executor/status correction). This
can corrupt the original completion time. Prefer only setting `finishedAt` when
the job becomes terminal *and* the existing `finishedAt` is not already set (<=
0), otherwise preserve the existing value.
##########
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:
`finishedAt` is overwritten with `Instant.now()` on *any* transition where
`newStatus` is terminal, even if the job already had a positive `finishedAt`
(e.g., a terminal→terminal transition due to executor/status correction). This
can corrupt the original completion time. Prefer only setting `finishedAt` when
the job becomes terminal *and* the existing `finishedAt` is not already set (<=
0), otherwise preserve the existing value.
--
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]
