DanielLeens commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5601184777
Thanks @SEZ9 — fair ask, a summary conclusion isn't the same as a pointer you can verify yourself. I re-pulled `2dd45e26f8613e0ee682d48aed9e963fc1d004f7` again and confirmed each of these against the actual current file contents (not my notes): 1. **`rows` validation + overflow guard** — `PageBaseServlet.java:107-124` (`pageParams()`): `rows` goes through the same `positiveIntParam()` helper as `page` (`:112-114`), and the offset is computed as `long start = (long) (page - 1) * rows;` with an explicit `start > Integer.MAX_VALUE` rejection (`:119-122`) before narrowing to `int`. `positiveIntParam()` itself is `:126-139`. 2. **`start == total` boundary, kept intentional** — `PageBaseServlet.java:141-152` (`checkPageInRange()`), with a doc comment on the method (`:142-146`) explicitly recording that a page starting exactly at `total` returns an empty page rather than being rejected, and why (matches legacy `writeJsonWithPagination` behavior). 3. **Duplication folded together** — `PageBaseServlet.java:48-70` (`writeJsonWithPagination()`): it now calls `pageParams(req)` (`:53`), `checkPageInRange(pageParams, total)` (`:58`), and `writeJsonPage(...)` (`:69`) instead of having its own parse/validate logic. This is the same method `RunningJobsServlet` calls for `/running-jobs` and `/running-jobs/summary`, so both endpoints now go through the identical validation path. 4. **`JobInfoService` preconditions** — `JobInfoService.java:135-146` (`getJobsByStateJson(String state, int start, int rows)`): explicit `state == null`, `start < 0`, `rows < 1` checks, each with a message naming the argument, before `matchingJobStates(state)` runs. 5. **Docs** — `docs/en/engines/zeta/rest-api-v2.md:786-795` states the `page`/`rows` constraints and the three 400 conditions (non-integer, non-positive, out-of-range) plus the `start == total` empty-page carve-out; `docs/zh/engines/zeta/rest-api-v2.md` carries the same in Chinese. `docs/en/introduction/concepts/incompatible-changes.md:6-12` (and the `zh` equivalent) has a `## dev` entry naming all three affected endpoints, `GET /finished-jobs/:state`, `GET /running-jobs`, and `GET /running-jobs/summary` — I checked `JettyService.java` again and confirmed the latter two really do share `RunningJobsServlet`, so item 3 above is what makes this entry accurate rather than aspirational. 6. **Servlet-level tests** — new file `seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/rest/servlet/FinishedJobsServletTest.java`, plus the package-private `FinishedJobsServlet(NodeEngineImpl, JobInfoService)` constructor at `FinishedJobsServlet.java:41-44` that makes it testable without a running node engine. 7. **`matchingJobStates()` cost documented as a follow-up** — PR description, the paragraph beginning "One cost this PR does not remove, for the record." It's not in a source comment, just the description; say the word if you'd rather it also live as a code comment on `JobInfoService.matchingJobStates()` (`:164`) so it survives a squash-merge losing the description. On CI: no change since my last comment. I re-checked the fork directly (`awsomesud347/seatunnel` run `34089819303`) rather than trusting the apache-side pointer — it's still `cancelled`, attempt 4, same as when I last looked, with no newer run triggered. So we're still waiting on the `kudu-connector-it` 90-minute-budget issue the author described to actually get resolved with a clean run; nothing here changes my "approval stands, conditioned on a clean Build" position from before. -- 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]
