SEZ9 commented on PR #11819: URL: https://github.com/apache/seatunnel/pull/11819#issuecomment-5650321307
Thanks for the docs clarification in 2512a2284 — the distinction you've drawn (pre-read disappearance is discarded without advancing the offset; permission and other I/O errors fail the task; post-read identity changes fail without acknowledgement) is a sensible contract to document, and it matches the current behavior of the reader as I understand it. That said, since this commit is documentation only, the underlying points from the previous review remain open. Concretely: - **F1 / F2 / F5 (post-read identity check):** Documenting that a post-read identity change fails the task is fine as a statement of current behavior, but the concern was that a routine rotation/deletion during a read takes down the whole reader rather than being isolated to that split, and that rows from the replacing file may already have reached the sink before the check fires. I understand the "emitted rows cannot be retracted" reasoning — could you say whether you're planning any code change here (e.g. deferring emission until after the identity check, or isolating the failure to the split), or whether you're proposing to keep the current behavior and rely on the docs? Either is a legitimate position, but I'd like it stated explicitly so the decision is on record. - **F6 (only pre-read `NoSuchFileException` tolerated):** The docs now say other I/O errors fail the task. That answers "what happens", but not the robustness concern that a transient error on one tailed file crash-loops the whole streaming job. Is that the intended final behavior? - **F3 (serialized field growth in `FileSplitFinishedEvent`, `FileSourceSplit`, `FileSourceState`):** Still need confirmation that checkpoint/state restore from a pre-upgrade job has been verified. - **F4 (identity string from `fileKey.toString()`):** Still open — inode reuse / remount instability across restore isn't addressed by the docs change. - **F7 (stale fingerprint on skipped tail splits) and F8 (`processedBytes` / `getLastReadBytes()` sentinel contract):** No change yet as far as I can tell from this commit; a short note on each would help. Once CI on the new head finishes, please ping me and I'll take another look. If you'd prefer to split the behavioral changes into a follow-up PR and land the docs + current behavior here, say so and we can scope accordingly. <!-- streview-comment:1005 --> -- 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]
