SEZ9 commented on PR #12299: URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5842372422
Thanks for the follow-up on `50567aa788b0`. Going through the points from the previous round: **F1 (single read path / charset) and F3 / F4 (truncation notice)** The latest review describes `FileUtils.FileTail` now carrying the retained bytes, the size snapshot and the truncation decision from one open channel, with `LogContentReader` no longer making a second `Files.size(path)` call. If that is what the diff does, it would address both points, including the window at `173c053a` where a file growing between the size check and a second whole-file read could be read in full. I haven't traced this against the diff myself, so two quick confirmations would help: - Do both outcomes (truncated and not) now decode the retained bytes as UTF-8 through the same code path? - Is the truncation notice derived only from that single size snapshot, so it can't disagree with the returned content? **Still open — please confirm or push a follow-up** 1. **Compatibility record (F2 / F4):** the default 64 MB tail changes the response of the v1 and v2 log-content endpoints for existing deployments. Please add an entry to `incompatible-changes.md` for `seatunnel.engine.http.log-response-max-size-mb` (default, `<= 0` = unlimited, and that the response now carries a truncation notice). 2. **v1 REST docs (F6):** `rest-api-v1.md` (en and zh) describe the same `/log/<name>` and `/logs/<name>` endpoints and need the same note as the v2 page. 3. **Docs wording (F7):** please phrase the truncated size in terms of the configured option rather than a hard-coded "last 64 MB", and remove the stray double backtick above the new note in the zh page. 4. **Duplicated conversion (F8):** the MB-to-bytes conversion and the `<= 0` sentinel check were duplicated in `LogBaseServlet` and `LogService`. If that's already consolidated, a pointer to where is enough; otherwise a small shared helper would do. 5. **Extra copies in `tailFromLineStart` (F5):** could you say briefly how many copies of the retained tail the current `FileTail` path holds at peak, so the memory bound stated in the description matches the code? Once 1–3 are in and the rest are confirmed, I'm happy to approve. <!-- streview-comment:1320 --> -- 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]
