SEZ9 commented on PR #12299: URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5923564015
@CryoThrust thanks for the ping — I'll do a final pass on 2209591b1a9c. Before I sign off, a few of the earlier review points still need an explicit confirmation on this head, since the automated re-read describes the new shape (shared `LogContentReader`, UTF-8-safe line-aligned tail, one-line truncation notice) but doesn't spell everything out: 1. **Charset consistency (F1)** – the truncated tail is now decoded as UTF-8. Is the non-truncated path (`FileUtils` / `LogContentReader.read`) also decoding with an explicit UTF-8 charset, or does it still go through platform-default `readFileToStr`? The same file must decode the same way regardless of size. 2. **Truncation marker (F3/F4)** – good to see a truncation notice is now prepended. Please confirm it is emitted on both the v2 servlet path and the v1 `RestHttpGetCommandProcessor` path, and that it is covered by the REST IT (a short pointer to the assertion is enough). 3. **Incompatible change + v1 docs (F2/F4/F6)** – the default of 64 MB changes existing behaviour of the v1 and v2 log endpoints. I still need to see an entry in `incompatible-changes.md` and the option/behaviour documented in `rest-api-v1.md` (en and zh), not only the v2 page. If this landed in the rebase, point me to it. 4. **Docs wording (F7)** – please make sure the docs refer to the configured `seatunnel.engine.http.log-response-max-size-mb` value rather than hard-coding "last 64 MB", and drop the stray double backtick in the zh page. 5. **Extra copies (F5)** – the description says the tail is "decoded straight into the builder", which sounds like the second copy in `tailFromLineStart` is gone. Can you confirm how many times the retained window is materialised per request now? 6. **Duplicated conversion (F8)** – is the MB-to-bytes / `<= 0` sentinel logic now in a single place (`HttpConfig.getLogResponseMaxSizeBytes()`), with `LogBaseServlet` and `LogService` both delegating to it? Nothing above is new scope — it's just closing the loop on the earlier findings against the current head. Once you confirm (or push the small follow-ups for 3 and 4), I'm happy to approve and merge. <!-- streview-comment:1438 --> -- 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]
