This is an automated email from the ASF dual-hosted git repository. jerryshao pushed a commit to branch feat/job-list-time-filter-sort in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 22fdcd96595181b30f1bb6ee368f170659ea37cf Author: Jerry Shao <[email protected]> AuthorDate: Thu Aug 20 17:49:55 2026 +0800 [#12521] fix(job): Address round-2 review comments on listJobs - De-flake testListJobs by using fixed queuedAt values instead of back-to-back Instant.now() calls. - Parse queuedAfter/startedAfter/finishedAfter with OffsetDateTime instead of Instant so RFC 3339 numeric offsets parse consistently across JDKs, matching the OpenAPI format: date-time. - Treat blank sortBy/sortOrder/time-filter query params as "not set" instead of erroring, since @DefaultValue only applies when a param is absent. - Document that statusCounts is computed after time filtering. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- docs/open-api/jobs.yaml | 4 ++- .../gravitino/server/web/rest/JobOperations.java | 19 ++++++++--- .../server/web/rest/TestJobOperations.java | 39 +++++++++++++++++++--- 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/docs/open-api/jobs.yaml b/docs/open-api/jobs.yaml index 93dbb251c2..ab4f692f0b 100644 --- a/docs/open-api/jobs.yaml +++ b/docs/open-api/jobs.yaml @@ -834,7 +834,9 @@ components: type: object description: >- The number of jobs in "jobs", keyed by lower-case status name. Every status is - present, even at zero. + present, even at zero. Counted after queuedAfter/startedAfter/finishedAfter filtering, + so a time filter can make some statuses structurally zero - e.g. startedAfter always + yields queued: 0, since a queued job has no startedAt yet. additionalProperties: type: integer format: int64 diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java index eb9fa43f16..55f8b578e8 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java @@ -21,8 +21,10 @@ package org.apache.gravitino.server.web.rest; import com.codahale.metrics.annotation.ResponseMetered; import com.codahale.metrics.annotation.Timed; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; import com.google.common.collect.Lists; import java.time.Instant; +import java.time.OffsetDateTime; import java.time.format.DateTimeParseException; import java.util.Collections; import java.util.Comparator; @@ -310,11 +312,16 @@ public class JobOperations { try { // Parse/validate query params up front so a bad request fails fast, before paying for the - // dispatcher fetch and authorization filtering below. + // dispatcher fetch and authorization filtering below. @DefaultValue only applies when a + // param is absent, not when a client sends it empty (e.g. "?sortBy=&sortOrder="), so blank + // values are treated as "not set" here too. Instant queuedAfterInstant = parseInstant("queuedAfter", queuedAfter); Instant startedAfterInstant = parseInstant("startedAfter", startedAfter); Instant finishedAfterInstant = parseInstant("finishedAfter", finishedAfter); - Comparator<JobEntity> comparator = buildJobComparator(sortBy, sortOrder); + Comparator<JobEntity> comparator = + buildJobComparator( + Strings.isNullOrEmpty(sortBy) ? "queuedAt" : sortBy, + Strings.isNullOrEmpty(sortOrder) ? "desc" : sortOrder); return Utils.doAs( httpRequest, @@ -548,11 +555,15 @@ public class JobOperations { @VisibleForTesting static Instant parseInstant(String paramName, String value) { - if (value == null) { + if (Strings.isNullOrEmpty(value)) { return null; } try { - return Instant.parse(value); + // OffsetDateTime.parse (RFC 3339, matching the OpenAPI `format: date-time`) rather than + // Instant.parse (strict ISO_INSTANT, `Z`/zero-offset only): Instant.parse of a numeric + // offset like "+08:00" throws on JDK 8/11 and only started accepting it on JDK 12+ + // (JDK-8166138), so the same request would 400 or succeed depending on the server's JDK. + return OffsetDateTime.parse(value).toInstant(); } catch (DateTimeParseException e) { throw new IllegalArgumentException( "Invalid " diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java index d3c778d2b9..130fae9434 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestJobOperations.java @@ -643,11 +643,14 @@ public class TestJobOperations extends JerseyTest { @Test public void testListJobs() { String templateName = "shell_template_1"; - JobEntity job1 = newJobEntity(templateName, JobHandle.Status.QUEUED); + // Fixed, strictly-increasing queuedAt values rather than back-to-back Instant.now() calls - + // the latter can collide (millisecond clock resolution on some JDKs/OSes), which would make + // the desc-sort assertions below flaky. + JobEntity job1 = newJobEntityWithQueuedAt(templateName, JobHandle.Status.QUEUED, 1000L, 0L, 0L); JobEntity job2 = - newJobEntity(templateName, JobHandle.Status.STARTED, Instant.now().toEpochMilli(), 0L); + newJobEntityWithQueuedAt(templateName, JobHandle.Status.STARTED, 2000L, 2500L, 0L); JobEntity job3 = - newJobEntity("spark_template_1", JobHandle.Status.SUCCEEDED, Instant.now().toEpochMilli()); + newJobEntityWithQueuedAt("spark_template_1", JobHandle.Status.SUCCEEDED, 3000L, 0L, 3500L); when(jobOperationDispatcher.listJobs(metalake, Optional.empty())) .thenReturn(Lists.newArrayList(job1, job2, job3)); @@ -675,8 +678,8 @@ public class TestJobOperations extends JerseyTest { 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. + // Default sort is queuedAt desc (newest first); job1/job2/job3 have strictly increasing + // queuedAt (1000L < 2000L < 3000L), so the response reverses that order. Assertions.assertEquals(3, jobListResponse.getJobs().size()); Assertions.assertEquals(JobOperations.toDTO(job3), jobListResponse.getJobs().get(0)); Assertions.assertEquals(JobOperations.toDTO(job2), jobListResponse.getJobs().get(1)); @@ -881,6 +884,25 @@ public class TestJobOperations extends JerseyTest { ErrorResponse errorResp5 = resp5.readEntity(ErrorResponse.class); Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE, errorResp5.getCode()); Assertions.assertEquals(IllegalArgumentException.class.getSimpleName(), errorResp5.getType()); + + // Blank query params (e.g. "?sortBy=&sortOrder=&queuedAfter="), as templated/generated + // clients sometimes send, fall back to "not set" rather than 400ing: @DefaultValue only + // applies when a param is absent, not when it's present-but-empty. + Response resp6 = + target(jobRunPath()) + .queryParam("sortBy", "") + .queryParam("sortOrder", "") + .queryParam("queuedAfter", "") + .request(APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp6.getStatus()); + JobListResponse jobListResponse6 = resp6.readEntity(JobListResponse.class); + Assertions.assertEquals(3, jobListResponse6.getJobs().size()); + Assertions.assertEquals(JobOperations.toDTO(job1), jobListResponse6.getJobs().get(0)); + Assertions.assertEquals(JobOperations.toDTO(job2), jobListResponse6.getJobs().get(1)); + Assertions.assertEquals(JobOperations.toDTO(job3), jobListResponse6.getJobs().get(2)); } @Test @@ -1003,11 +1025,18 @@ public class TestJobOperations extends JerseyTest { @Test public void testParseInstant() { Assertions.assertNull(JobOperations.parseInstant("queuedAfter", null)); + Assertions.assertNull(JobOperations.parseInstant("queuedAfter", "")); Instant expected = Instant.ofEpochMilli(1500L); Assertions.assertEquals( expected, JobOperations.parseInstant("queuedAfter", expected.toString())); + // RFC 3339 numeric offsets (as advertised by the OpenAPI `format: date-time`) are accepted, + // not just the strict ISO_INSTANT `Z` form. + Assertions.assertEquals( + Instant.parse("2026-08-17T16:00:00Z"), + JobOperations.parseInstant("queuedAfter", "2026-08-18T00:00:00+08:00")); + Assertions.assertThrows( IllegalArgumentException.class, () -> JobOperations.parseInstant("queuedAfter", "not-a-timestamp"));
