SEZ9 commented on PR #12299: URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5903186076
@CryoThrust thanks for the merge-readiness summary on `2209591b1a9c`. Before I sign off, I'd like to close the loop on the points from my earlier review, since I can't see from the thread that they've been addressed on this head. Could you confirm the status of each, or point me to where they were resolved? 1. **Charset consistency** – the same log file is decoded via the platform-default `readFileToStr` when it's within the limit but as UTF-8 when it's over the limit. Both paths should use the same explicit charset. 2. **Truncation marker** – a truncated response currently carries no indication that the head of the file was dropped, so the Web UI iframe and API consumers can't distinguish a full log from a tail. 3. **Compatibility record** – the default 64 MB tail changes the existing behaviour of both the v1 and v2 log-content endpoints for every deployment. Please add an entry to `incompatible-changes.md`. 4. **v1 REST docs** – the v1 endpoints are now bounded too, but `rest-api-v1.md` (en/zh) is not updated; the option is documented only on the v2 page. 5. **Docs wording** – the note hard-codes "last 64 MB" rather than referring to the configured value, and the zh page has a stray double backtick right above the new note. 6. **Duplicated conversion** – the MB-to-bytes conversion and the `<=0` sentinel are repeated verbatim in `LogBaseServlet` and `LogService`; a single shared helper would be cleaner. 7. **Memory bound** – `tailFromLineStart` copies the retained window a second time, so per request the tail is materialised several times on the heap and the actual bound is looser than the PR claims. If you'd rather keep that as a follow-up, please at least adjust the PR description so it doesn't overstate the guarantee. Items 1–5 are the ones I'd like fixed in this PR; 6 and 7 I'm fine deferring if you'd prefer, but please say so explicitly. Once those are in (or you point me to where they already landed), I'll do the final pass. <!-- streview-comment:1420 --> -- 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]
