DanielLeens commented on PR #12299: URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5775114848
Thanks for checking in on `5564bd74f6ba` - good timing, my review just posted on this same head covers exactly these points with pointers. Quoting the relevant bits back here so you don't have to dig for them: - F3/F4 (truncation marker): `LogContentReader.java:39-42` (notice format) and `:75` (prepended when `size > limit`); wired into both endpoints at `LogBaseServlet.java:76-77` (v2) and `RestHttpGetCommandProcessor.java:420-421` (v1). `LogContentReaderTest.readAnnouncesThatALargeFileWasTruncated` and `readReturnsAFileWithinTheLimitUnchanged` cover the present/absent cases; the exact-at-limit boundary is only asserted at the `FileUtilsTest` level. - F8 (duplicated MB-to-bytes / `<= 0` sentinel): consolidated in `HttpConfig.getLogResponseMaxSizeBytes()` (`HttpConfig.java:100-101`); both `LogService.maxLogResponseBytes()` and `LogBaseServlet.maxLogResponseBytes()` just delegate now. Closed. - F1 (charset): all three read paths decode UTF-8 through `readFileToUtf8Str` - `FileUtils.java:115` (unlimited), `:121` (fits), `:131` (tail), helper at `:152-154`. Closed. - F5 (extra copies): a single `ByteBuffer` read (`FileUtils.java:123-129`), `lineStartOffset` returns an offset (`:160-177`), one `new String(...)` decode (`:131`) - no more `copyOfRange`. Closed. - F2/F6 (incompatible-changes.md + v1 docs): `docs/en/introduction/concepts/incompatible-changes.md:335-352` / zh `:299-312`; v1 docs at `docs/en/engines/zeta/rest-api-v1.md:1007-1037` / zh `:1009-1040`. - F7 (doc wording, stray backtick): `rest-api-v2.md` now names the option instead of a hard-coded size (`:1455-1471`), and the stray backtick in the zh page is gone. So all eight of your points (F1-F8) are resolved at this head - none of them are a blocker from my side, and you don't need another push for any of them. The scope question is the one still open, and it's the only thing keeping my conclusion at "not recommended for merge": since my last review the branch picked up four more commits unrelated to the log change (`7e6afc6194` Postgres CDC backport, `5564bd74f6` Zeta task cancellation, `0e735b4f8f` PayPal test, `772cc9891c` FilterRowKind E2E), on top of the earlier #12311/#12313/#12346 copies. That's Issue 1 in the full review, raised to High severity this round. My recommendation is the same one I gave on #12298: rebase onto current `dev` so this PR carries only its own three commits (`4928aee047`, `254614a70d`, `9531cecd4e`), and let the rest land through their own PRs. Once that's done I expect to approve after Issue 2 (the non-atomic size snapshot deciding the truncation notice) and Issue 3 (a wiring test for the limit) are addressed or explicitly deferred. -- 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]
