DanielLeens commented on PR #12299:
URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5846326075
Thanks for double-checking these — good news is there's nothing outstanding
here. This exact head (`50567aa788b0`) already carries my APPROVED review from
yesterday, which re-verified all six follow-up items directly against the diff
rather than taking the write-up at face value. Let me answer each point below
directly, re-confirmed against the current head just now rather than repeating
what I said before:
**F1 / F3 / F4 (single snapshot, single decode path):** Yes to both
questions. `FileUtils`'s tail read now takes one `channel.size()` snapshot and
hands it back inside `FileTail` together with the retained bytes and the
truncation flag; `LogContentReader.read` no longer calls `Files.size(path)` a
second time, so the notice can never disagree with what was actually returned.
Both the "fits in the limit" and the tail branch decode through the same UTF-8
path, so the same file is never decoded two different ways depending only on
its size.
**F2 / F6 (compatibility record + v1 docs):** Both present.
`incompatible-changes.md` (en/zh) has an entry describing the default 64 MB
tail and the `log-response-max-size-mb: 0` opt-out. `rest-api-v1.md` (en/zh)
has its own "Response Size Limit" section covering both `/log/<name>` and
`/logs/<name>`, cross-linking the v2 page instead of leaving the option
documented only there.
**F7 (doc wording):** Fixed. Both pages describe the limit in terms of the
configured option ("at most `seatunnel.engine.http.log-response-max-size-mb` of
content, 64 MB by default") rather than a hardcoded "last 64 MB", and the stray
double backtick on the zh page is gone.
**F8 (duplicated conversion):** Consolidated.
`HttpConfig.getLogResponseMaxSizeBytes()` is the single place that turns the
configured MB value into a byte count (`<= 0` maps to unlimited);
`LogBaseServlet` and `LogService` each just delegate to it now.
**F5 (extra copies):** Down to one. The truncated path decodes the retained
window straight into the notice-prefixed response — there's no longer an
intermediate full `String` built and then concatenated onto the notice.
So there's nothing left for me on the code side — this head is already at
"ready to merge" from my last review, with only one new, non-blocking Low item
of my own on top (a rotated/deleted-file edge case in the notice wording, not
something that needs another push). The remaining blocker is still CI: `Build`
on this head is red, but it's the same story as before — the failures are in
unrelated areas this diff never touches (the checkpoint/backpressure flakes
tracked against #12313/#12449, plus the OceanBase serialization and
Databend/MinIO image-pull issues you diagnosed). Once one of those clears or
this rebases past it, this is good to merge from my side.
--
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]
