jerryshao commented on code in PR #12522:
URL: https://github.com/apache/gravitino/pull/12522#discussion_r3820453046
##########
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:
Good catch -- fixed. `testListJobs` now uses `newJobEntityWithQueuedAt` with
fixed, strictly-increasing values (1000L/2000L/3000L) instead of back-to-back
`Instant.now()` calls, matching what `testListJobsWithTimeFiltersAndSort`
already does.
##########
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:
Yes, intentional. Previously the order was whatever the dispatcher happened
to return, which was never a documented contract - defaulting to `queuedAt
desc` gives callers a stable, useful order (newest first) instead of an
arbitrary one. The PR description's "no changes to existing fields or client
APIs" was about the DTO/API surface, not response ordering; agreed it's a
behavior change worth calling out explicitly, which the new sortBy/sortOrder
parameter docs and `testListJobs`'s comment now do. Open to leaving the list
unsorted-by-default instead if you feel strongly, but I'd rather ship a
deterministic default.
##########
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:
Good catch -- switched to `OffsetDateTime.parse(value).toInstant()`, which
handles RFC 3339 numeric offsets consistently across JDKs and matches the
`format: date-time` the spec advertises. Added a test asserting `+08:00`-style
offsets parse correctly.
--
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]
