SEZ9 commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5578282178
@DanielLeens thanks for re-pulling `2dd45e26f86` (`2dd45e26f8613e0ee682d48aed9e963fc1d004f7`) into a clean worktree and re-reading the changed files end to end. Your problem/fix summary matches my reading: the old `GET /finished-jobs/:state?page=&rows=` path paid per-job metrics and DAG lookups for every job matching the state filter before slicing, and the new flow filters/sorts into a lightweight `List<JobState>` via `matchingJobStates`, slices the page, and only then runs the per-row lookups. Your comment appears to have been cut off after the "Fix approach" bullet. If there was a verdict or a question at the end, please re-post it so I do not have to guess. From my earlier review, here is what I still need confirmed at the current head: 1. `pageParams()` should validate `rows` the same way it validates `page`, and `PageParams.getStart()` should guard the multiplication so overflow cannot wrap to a small positive value and slip past `checkPageInRange`. Today invalid input can surface as an opaque JDK exception or silently serve the wrong page. 2. `checkPageInRange` uses `start > total`; please confirm whether a page starting exactly at `total` should be an empty page or a rejection, and that this matches the legacy `writeJsonWithPagination()` behavior used by the other paged servlets. 3. `pageParams()`/`checkPageInRange()` and `writeJsonWithPagination()` now duplicate parse/validate logic — either fold them together or note why they must stay separate. 4. `getJobsByStateJson(state, start, rows)` should have explicit precondition checks rather than relying on Stream internals to reject bad arguments. 5. REST API documentation and a release-note entry for the changed pagination semantics of `GET /finished-jobs/:state`. 6. Test coverage for the servlet-side surface (`pageParams`, `checkPageInRange`, `writeJsonPage`, and the `FinishedJobsServlet` wiring); `JobInfoServiceNullSafetyTest` currently only exercises the service overload. 7. A note in the PR description that `matchingJobStates()` still deserializes and sorts every retained `JobState` per request, so the remaining cost is not misread as fixed — fine as a follow-up. If any of these are already addressed in `2dd45e26f86`, a short pointer to where is all I need and I will re-check. <!-- streview-comment:889 --> -- 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]
