jerryshao commented on PR #13250: URL: https://github.com/apache/gravitino/pull/13250#issuecomment-5749472172
Hi @neoLsH, thanks so much for the thorough review — this is exactly the kind of feedback that's useful before merge. Here's where I landed on each point: **1. Byte-vs-line cap / O(S²) risk on a pathological single huge line** — you're right, this is a real gap and I'll fix it in this PR. Will bound the actual bytes read and returned (not just the line count), so a giant single line can't blow up the request thread or the response size. **2. Single atomic call + `truncated` flag vs. the current two separate `getJobStdout`/`getJobStderr` calls** — I don't think this rises to a real problem. Snapshotting stdout/stderr at slightly different moments for a still-running job doesn't meaningfully hurt debuggability in practice, and true atomicity isn't really achievable here anyway even with a single method — it would still mean two sequential file reads underneath. I'd rather keep the simpler two-method shape as-is. **3. Retention window mismatch (1h vs 7d)** — this is a limitation of the local job executor as it stands today, and I'm treating it as expected/known behavior rather than something to reconcile in this PR. **4. Multi-node ownership gap (non-owning node returns empty)** — also a local-job-executor-specific limitation. I'm planning to address this separately in a follow-up PR rather than folding it into this one. Thanks again for the careful read — will push the fix for #1 shortly. -- 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]
