SEZ9 commented on PR #11819:
URL: https://github.com/apache/seatunnel/pull/11819#issuecomment-5643011063
Thanks for the follow-up commit. Based on the thread, `922bb392` ("Clear
fingerprints for skipped tail splits") touches
`MultipleTableFileSourceReader.java`, adds Javadoc to `ReadStrategy.java` and
`FileSplitFinishedEvent.java`, extends
`MultipleTableFileSourceReaderTest.java`, and adds a sentence to the en/zh
docs. That lines up with two of my earlier points:
- **PR11819-F7 (stale fingerprint on skipped tail splits)** — the reader
change plus the new test appear to cover this. Could you confirm the test
asserts that a skipped tail split reports an empty/absent fingerprint in
`FileSplitFinishedEvent` rather than the value from the previous read?
- **PR11819-F8 (`processedBytes` / `getLastReadBytes()` sentinel contract)**
— the Javadoc additions to `ReadStrategy.java` look like they address this.
Please just confirm the doc spells out when `-1` vs `0` is returned so callers
don't have to infer it.
The remaining items from my previous review are not touched by `9fd60f30f5a`
→ `922bb39277c` as far as the thread describes, so I'd still like a response on
each:
- **PR11819-F1 / PR11819-F2** — the post-read identity re-check in
`MultipleTableFileSourceReader` still fails the whole reader when a
rotation/deletion happens mid-read, and rows are already emitted before that
check runs. Please either move the identity check ahead of emission (or buffer
until it passes) and downgrade the mismatch to a per-split skip, or explain why
failing the task is the intended behavior for tail mode.
- **PR11819-F3** — `FileSplitFinishedEvent`, `FileSourceSplit` and
`FileSourceState` all gained serialized fields. Please add (or point me to) a
restore test from a pre-upgrade checkpoint/state so we know upgrade of an
existing job is safe.
- **PR11819-F4 / PR11819-F5** — `LocalFileIdentity` uses
`fileKey.toString()` (dev+inode). Inode reuse and remount/reboot can make a
different file look identical, both at checkpoint restore and in the
reader-side TOCTOU window. Please describe how the content anchor mitigates
this, and whether the identity should combine fileKey with something more
stable (e.g. the anchor or creation time) rather than fileKey alone.
- **PR11819-F6** — only pre-read `NoSuchFileException` is tolerated; other
transient I/O errors on one tailed file will crash-loop the streaming job.
Please consider treating other `IOException`s on a single file as a
skip-and-retry for that file, or document why they must be fatal.
Once F1/F2 and F3 are addressed (or justified) I'm happy to take another
pass; F4–F6 can be follow-ups if you prefer, but I'd like that decision to be
explicit in the PR description.
<!-- streview-comment:983 -->
--
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]
