DanielLeens commented on PR #12299: URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5661994548
Thanks for the very thorough independent pass, @SEZ9 — this is complementary to my own review rather than overlapping with it, so let me reconcile the two. I spot-verified several of your findings against the current head (`12b99ec9`, same head my review was based on — no new commit has landed) and they hold up: - **Issue 1 (charset inconsistency)**: confirmed. `readFileToStr()` decodes with the platform-default charset (`FileUtils.java:76`, `new String(bytes)`), and both early-return paths of `readFileTailToStr` (`maxBytes <= 0` and `size <= maxBytes`) fall through to that same method, while the actual tail-read branch explicitly decodes UTF-8 (`FileUtils.java:117-118`). Same file, same endpoint, different decoding depending only on size — real bug. - **Issue 5 (extra copy in `tailFromLineStart`)**: confirmed, `Arrays.copyOfRange` at `FileUtils.java:127/136` is a second full-size allocation on top of the `ByteBuffer` and the resulting `String`. Your point about returning a start offset instead is the right fix. - **Issue 8 (duplicated `maxLogResponseBytes()`)**: confirmed byte-for-byte identical between `LogBaseServlet.java:88-96` and `LogService.java:55-63`. - **Issues 2/4/6 (missing `incompatible-changes.md` entry, missing v1 docs)**: confirmed — I grepped both files and neither has an entry for this option. None of these overlap with the two High-severity issues @goutamadwant and I already flagged and that are still open on this same head: the `size <= maxBytes` fast path still calls the fully-unbounded `readFileToStr()` (so a generous-but-supported `log-response-max-size-mb` above ~2GB reintroduces the exact OOM this PR is meant to prevent, for files between 2GB and that limit), and `tailFromLineStart` returns an empty string instead of the tail when the retained window's only `\n` is the file's own trailing terminator. So between your list and mine, this PR currently has 2 High + up to 8 Medium/Low open items, none of which are addressed by anything already pushed. No new commit has landed since my last review, so I'm not doing a fresh full pass right now — but wanted to confirm your findings are real and get the combined picture on record. Once a fix lands covering both sets, I'll do a full re-review of the new head. CI (`Build`) is still in progress as of this comment, nothing new to report there yet. -- 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]
