SEZ9 commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5628396906
@awsomesud347 Thanks for laying this out against the F numbering, and apologies for the delay in getting back to you. On **F1 / F5**: the approach you describe — `rows` going through the same `positiveIntParam()` helper as `page`, the offset computed as `long` and rejected when it exceeds `Integer.MAX_VALUE` before narrowing, and `PageParams.getStart()` returning a value already validated at construction — is what I was asking for. I'll confirm it against `PageBaseServlet.java:107-124` and `:126-139` on `2dd45e26f`, and based on your description I expect to resolve both. On **F2** and the remaining items: your comment appears to have been cut off mid-sentence, so I can't see the rest of the F2 parity argument or any F3–F8 entries here. I'll go through the per-item answers you linked (5578063614, 5578311990, 5601184777), but if the rest of this comment was meant to add anything beyond those, could you repost it? In particular: - **F2**: confirmation that `start > total` matches the existing `writeJsonWithPagination` check rather than tightening or loosening it. - **F3**: where the REST doc / release-note update for the `GET /finished-jobs/:state` pagination change landed. - **F4**: whether the other paged servlets now share the new helpers, or if the duplication is intentionally left for a follow-up. - **F6**: where the precondition checks on `getJobsByStateJson(state, start, rows)` live. - **F7**: whether servlet-level tests for `pageParams` / `checkPageInRange` / `writeJsonPage` were added, or if coverage is still only via the service overload. - **F8**: a one-line confirmation that this is tracked as a follow-up is enough. Once I have that, I'll check each location on `2dd45e26f` and close out the review. <!-- streview-comment:952 --> -- 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]
