SEZ9 commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5612221534
Thanks @awsomesud347 for the targeted rerun and @DanielLeens for verifying run 34089819303 and the `Build` result on `2dd45e26f` directly — good to have a clean `Build` on the exact reviewed commit (`2dd45e26f8613e0ee682d48aed9e963fc1d004f7`), and tracing `kudu-connector-it (11)` to the job logs rather than writing it off as flakiness was the right call. Since the recent comments only cover CI, I'd like to re-check the points from my earlier review against this commit before we merge. I may well have missed changes, so please point me at anything already addressed: - **F1 / F5 (`pageParams()` / `PageParams.getStart()`)**: is `rows` validated alongside `page`, and is the start computation protected against `int` overflow (which could either surface an opaque JDK exception or wrap to a small positive value that slips past `checkPageInRange`)? - **F2 (`checkPageInRange`)**: with `start > total`, a page starting exactly at `total` returns an empty page rather than being rejected — does that match the legacy `writeJsonWithPagination` behavior? - **F4**: pagination parsing/validation appears duplicated between `pageParams()`/`checkPageInRange()` and `writeJsonWithPagination()`; could the paged servlets share one path, or at least the validation? - **F6 (`getJobsByStateJson(state, start, rows)`)**: are there explicit precondition checks, or does argument validation still fall through to Stream internals? - **F3 (`FinishedJobsServlet`)**: the `GET /finished-jobs/:state` pagination semantics change (source-side slicing, out-of-range behavior) — is there a REST API documentation update for this? - **F7 (tests)**: `JobInfoServiceNullSafetyTest` exercises the service overload; is the servlet path (`pageParams`, `checkPageInRange`, `writeJsonPage`, `FinishedJobsServlet` wiring) covered by existing REST API IT/E2E tests, or would a targeted test be worth adding? - **F8 (`matchingJobStates()`)**: LOW — happy to track this as a follow-up if you'd prefer. None of these are meant to reopen the CI discussion; I just want to confirm their status on this commit so we merge with a clear picture. <!-- streview-comment:947 --> -- 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]
