yuqi1129 commented on code in PR #12522:
URL: https://github.com/apache/gravitino/pull/12522#discussion_r3820268729
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -285,13 +291,31 @@ public Response alterJobTemplate(
public Response listJobs(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
String metalake,
- @QueryParam("jobTemplateName") String jobTemplateName) {
+ @QueryParam("jobTemplateName") String jobTemplateName,
+ @QueryParam("queuedAfter") String queuedAfter,
+ @QueryParam("startedAfter") String startedAfter,
+ @QueryParam("finishedAfter") String finishedAfter,
+ @QueryParam("sortBy") @DefaultValue("queuedAt") String sortBy,
Review Comment:
This changes what existing callers get back.
`sortBy` defaults to `queuedAt` and `sortOrder` to `desc`. So `GET
/metalakes/{m}/jobs/runs` with no query parameters now returns jobs
newest-first, where before it returned them in whatever order the dispatcher
gave.
The PR description says "No changes to existing fields or client APIs", but
the assertions in `testListJobs` and `TestSupportsJobs` had to be flipped,
which shows the order on the wire did change. Anything that reads the list by
position - a UI treating `jobs[0]` as the oldest job, for example - will
silently get the opposite result after an upgrade.
Is the new default order intended? If keeping the old behaviour matters, the
list could stay unsorted unless `sortBy` is actually passed.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java:
##########
@@ -661,28 +664,41 @@ public void testListJobs() {
JobListResponse jobListResponse = resp.readEntity(JobListResponse.class);
Assertions.assertEquals(0, jobListResponse.getCode());
+ // statusCounts reflects the returned jobs: one QUEUED, one STARTED, one
SUCCEEDED, and every
+ // other status present at zero.
+ Map<String, Long> expectedStatusCounts = new HashMap<>();
+ expectedStatusCounts.put("queued", 1L);
+ expectedStatusCounts.put("started", 1L);
+ expectedStatusCounts.put("failed", 0L);
+ expectedStatusCounts.put("succeeded", 1L);
+ expectedStatusCounts.put("cancelling", 0L);
+ expectedStatusCounts.put("cancelled", 0L);
+ Assertions.assertEquals(expectedStatusCounts,
jobListResponse.getStatusCounts());
+
+ // Default sort is queuedAt desc (newest first); job1/job2/job3 were
created in that order,
+ // so queuedAt increases job1 < job2 < job3 and the response reverses it.
Assertions.assertEquals(3, jobListResponse.getJobs().size());
- Assertions.assertEquals(JobOperations.toDTO(job1),
jobListResponse.getJobs().get(0));
+ Assertions.assertEquals(JobOperations.toDTO(job3),
jobListResponse.getJobs().get(0));
Review Comment:
This assertion can fail at random.
`job1`, `job2` and `job3` are built with `newJobEntity(...)`, which sets
`createTime` to `Instant.now()` (line 1226). `queuedAt` reads that same field
(`JobOperations.java:590`). So this test needs the three `Instant.now()` calls
to return three different values.
They often will not. On JDK 8 `Instant.now()` only has millisecond
precision, and on Windows the clock ticks about every 15 ms. When two of the
three land on the same instant, the comparator returns 0. `filterAndSortJobs`
sorts with `Stream.sorted`, which is stable, so those jobs come back in
insertion order and this line gets `job1` instead of `job3`.
`testListJobsWithTimeFiltersAndSort` in this same PR already does the right
thing: it uses `newJobEntityWithQueuedAt(..., 3000L, ...)` with fixed values.
Could `testListJobs` use that helper too, instead of relying on the wall clock
to space the jobs apart?
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -485,4 +517,117 @@ static JobDTO toDTO(JobEntity jobEntity) {
private static List<JobDTO> toJobDTOs(List<JobEntity> jobEntities) {
return
jobEntities.stream().map(JobOperations::toDTO).collect(Collectors.toList());
}
+
+ @VisibleForTesting
+ static Map<String, Long> countJobsByStatus(List<JobEntity> jobEntities) {
+ // Every status is present, even at zero, so callers get a stable set of
keys to render
+ // (e.g. a status histogram) without having to special-case missing
entries.
+ Map<String, Long> statusCounts = new LinkedHashMap<>();
+ for (JobHandle.Status status : JobHandle.Status.values()) {
+ statusCounts.put(status.name().toLowerCase(Locale.ROOT), 0L);
+ }
+ for (JobEntity jobEntity : jobEntities) {
+ statusCounts.merge(jobEntity.status().name().toLowerCase(Locale.ROOT),
1L, Long::sum);
+ }
+ return statusCounts;
+ }
+
+ @VisibleForTesting
+ static List<JobEntity> filterAndSortJobs(
+ List<JobEntity> jobEntities,
+ Instant queuedAfter,
+ Instant startedAfter,
+ Instant finishedAfter,
+ Comparator<JobEntity> comparator) {
+ return jobEntities.stream()
+ .filter(
+ jobEntity -> matchesTimeFilters(jobEntity, queuedAfter,
startedAfter, finishedAfter))
+ .sorted(comparator)
+ .collect(Collectors.toList());
+ }
+
+ @VisibleForTesting
+ static Instant parseInstant(String paramName, String value) {
+ if (value == null) {
+ return null;
+ }
+ try {
+ return Instant.parse(value);
Review Comment:
`Instant.parse` accepts different input on different JDKs, and it does not
match what the spec promises.
`Instant.parse` uses `ISO_INSTANT`. The OpenAPI spec declares these
parameters as `type: string, format: date-time`, which is RFC 3339 and allows a
numeric offset.
`Instant.parse("2026-08-18T00:00:00+08:00")` throws on JDK 8 and 11, so the
server answers 400. On JDK 12 and later the same string parses fine, because
`ISO_INSTANT` was widened (JDK-8166138). Gravitino runs on several JDKs, so the
same request succeeds or fails depending on which JDK the server happens to use.
`OffsetDateTime.parse(value).toInstant()` would behave the same everywhere
and match the spec. If only `Z` is meant to be accepted, then the spec should
say so instead of `format: date-time`.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -285,13 +291,31 @@ public Response alterJobTemplate(
public Response listJobs(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
String metalake,
- @QueryParam("jobTemplateName") String jobTemplateName) {
+ @QueryParam("jobTemplateName") String jobTemplateName,
+ @QueryParam("queuedAfter") String queuedAfter,
+ @QueryParam("startedAfter") String startedAfter,
+ @QueryParam("finishedAfter") String finishedAfter,
+ @QueryParam("sortBy") @DefaultValue("queuedAt") String sortBy,
+ @QueryParam("sortOrder") @DefaultValue("desc") String sortOrder) {
Review Comment:
Two smaller notes, both minor.
**Empty parameter values give a 400 instead of the default.** In JAX-RS
`@DefaultValue` only kicks in when the parameter is missing. If a client sends
`?sortBy=&sortOrder=` - which templated or generated clients often do -
`sortBy` is `""`, not `"queuedAt"`. That falls into the `default:` branch of
`buildJobComparator` and returns 400 with `Invalid sortBy value: `. The same
happens for `queuedAfter=`, `startedAfter=` and `finishedAfter=`, where
`Instant.parse("")` throws. Treating a blank value as "not set" would avoid
this.
**`statusCounts` counts only the jobs left after the time filters (line
340), which can look odd in a UI.** With `startedAfter` set, `queued` is always
0 - a queued job has no `startedAt`, so it is always filtered out. With
`finishedAfter` set, `queued`, `started` and `cancelling` are always 0 for the
same reason. The spec text ("The number of jobs in \"jobs\"") is accurate, but
the comment on `countJobsByStatus` says the counts exist so a UI can draw a
status histogram, and such a histogram would show zeros that are impossible by
construction whenever a time filter is used. Worth saying this explicitly in
the spec description, or counting before the time filters are applied.
--
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]
