awsomesud347 commented on PR #12130:
URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5578063614
@SEZ9 All seven are addressed as of `2dd45e26f`, in your numbering.
1. `pageParams()` now validates both parameters rather than only `page`. A
value that is not an integer produces a message naming the parameter instead of
the JDK's wording, and a value below 1 is rejected for both `page` and `rows`.
The start offset is computed as `(long) (page - 1) * rows` and rejected if it
exceeds `Integer.MAX_VALUE` before being narrowed, so the wrap you described
can no longer reach `checkPageInRange`. The validated offset is stored on
`PageParams` rather than recomputed on demand, so no accessor can throw.
2. No change, and it is intentional. The legacy method used `start > total
|| page < 1`, so a page starting exactly at `total` has always returned an
empty page rather than being rejected, and `checkPageInRange` preserves that
exactly. There is now a comment recording it so a later reader does not "fix"
it. If you would rather it rejected, that is a one line change.
3. Done. `writeJsonWithPagination` now calls `pageParams()`,
`checkPageInRange()` and `writeJsonPage()`, so there is a single definition of
pagination input handling. This means `/running-jobs` and
`/running-jobs/summary` pick up the same validation, so `rows=0` there now
returns 400 where it previously returned an empty page.
4. Done. `getJobsByStateJson(state, start, rows)` now rejects a null
`state`, a negative `start` and a `rows` below 1, each with a message naming
the argument, rather than relying on the stream pipeline to fail.
5. Done. The `page` and `rows` tables in
`docs/en/engines/zeta/rest-api-v2.md` and `docs/zh/engines/zeta/rest-api-v2.md`
now state the constraints and the 400 behaviour, and describe both the envelope
and the out of range semantics. There is also an entry in
`incompatible-changes.md` in both languages. Following @nzw921rx's review that
entry names `/running-jobs/summary` as well, since it is registered against the
same `RunningJobsServlet` instance and therefore receives the same validation.
6. Done. A new `FinishedJobsServletTest` adds nine tests covering zero and
negative `rows`, a non integer value, a `page` below 1, an offset that
overflows an int, a page starting beyond the end of the results, a page
starting exactly at `total`, and the routing between the paged and unpaged
service calls.
One thing to flag since you did not ask for it. Making the servlet
testable required a package private `FinishedJobsServlet(NodeEngineImpl,
JobInfoService)` constructor, following the existing `WorkerResourceServlet`
pattern, and widening `JobInfoService.JobPage`'s constructor from private to
public, because `JobPage` is a final class that Mockito cannot stub. Happy to
take a different approach if you would rather it stayed private.
7. Done, in the description, which now records that `matchingJobStates()`
still fetches, deserialises and sorts every retained `JobState` on each request
and that only the per row work became lazy.
@DanielLeens thanks for the re-read. On your three findings: Issue 2 is a
fair hit and mine, both the overflow rejection and the out of range check throw
the identical string "Page number exceeds total pages" (`PageBaseServlet` lines
121 and 150), so a client cannot tell which fired. Issue 1 I agree is
unreachable, since `checkPageInRange` rejects `start > total` before the
addition and `total` is a job count. Issue 3 is fair too. I have left all three
rather than spend a full CI cycle on them, but say the word and I will fix
Issue 2 in particular.
On CI. There are no test failures against this change anywhere. Across four
attempts every job has passed at least once: Azure Cosmos DB and Paimon cleared
on the second, RocketMQ on the third. The run still reports `cancelled` because
`kudu-connector-it (11)` has now hit its 90 minute limit (`backend.yml:1514`)
twice in a row, while the same tests pass on JDK 8 in 27 minutes. It has never
actually failed a test. I have stopped retrying rather than keep rolling the
same dice.
@nzw921rx flagging that for you since your approval was conditional on CI
passing. The diff is confined to `PageBaseServlet`, `JobInfoService` and
`FinishedJobsServlet`, and no E2E test in the repository references
`/finished-jobs` or `/running-jobs`, so nothing in the connector suites has a
code path to this change. Happy to rerun again if you would rather see a fully
green run.
Locally, `mvnw test -pl seatunnel-engine/seatunnel-engine-server` passes
with 470 tests, no failures and no errors, and `spotless:apply` and `mvnw
-DskipTests verify` are clean.
--
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]
