DanielLeens commented on PR #11819: URL: https://github.com/apache/seatunnel/pull/11819#issuecomment-5661998574
Thanks for confirming the duplicate-review housekeeping, @SEZ9. On the F1–F8 recap itself: I'd already answered all eight in detail in my `9444831a2` review (https://github.com/apache/seatunnel/pull/11819#pullrequestreview-5190454891), posted 2026-09-13T10:52:49Z — before this latest recap, so I want to make sure that answer doesn't get lost in the duplicate-review noise: - **F3, F7, F8 — closed with source-level evidence**, not just asserted: - F3: `FileSourceSerializationCompatibilityTest` compiles the pre-tailing shape of `FileSourceState`/`FileSplitFinishedEvent` at test time, serializes it, and deserializes through the current classes — plus an end-to-end legacy-state restore through a real enumerator (`ContinuousMultipleTableFileSourceSplitEnumeratorTest.testRestoresLegacyBinaryStateAndAcceptsLegacyFinishedEvent`). Pre-upgrade checkpoint restore is verified, not hand-waved. - F7: fixed a few commits back in `922bb3927`; I additionally checked that `contentFingerprint` is never read on the tailing-commit branch in the enumerator at all (`ContinuousMultipleTableFileSourceSplitEnumerator.java:379-414` uses `getEndContentAnchor()`, not the fingerprint), so there's no cross-branch staleness path even setting the null-clearing fix aside. - F8: `-1L` is the reader's pre-read placeholder; a skipped split explicitly sets `0L`; the enumerator falls back to the split's full length when `processedBytes < 0L` (`ContinuousMultipleTableFileSourceSplitEnumerator.java:381-384`) — legacy-compatible default confirmed by reading the consumer, not just the Javadoc. - **F4, F6 — deliberate, already-documented design positions, not open defects.** F4 (fileKey reuse/remount instability) is explicitly called out in `docs/en/connectors/source/LocalFile.md` as an accepted limitation of OS-provided file identity, with content-anchor sampling as a mitigation, not a guarantee. F6 (only pre-read `NoSuchFileException` tolerated) is confirmed-intentional: any other I/O failure fails the task today, consistent with how every other failure mode in this connector already behaves — recovery relies on the job's restart strategy, same as elsewhere. - **F1/F2/F5 — the one item I agree is a real, if narrow, trade-off**: a post-read identity mismatch fails the whole reader/subtask rather than isolating just that split, and rows read before the mismatch is detected can't be retracted. I don't think this blocks the PR — deferring emission until after a second identity check would mean unbounded per-split buffering, a materially bigger design change than this PR's scope — but I agree it's worth a maintainer's explicit sign-off rather than staying implicit. I'd record it as "current behavior, deliberate, not changing in this PR" rather than as an unresolved defect. Since `e21e0a67f040` (the only commit after that review) is a verified no-op merge for this PR's own files — I diffed all 16 files this PR owns against the previous head and only a benign, unrelated doc addition changed — there's no new commit to trigger a fresh technical re-derivation here. My answer above still stands as current. If any specific point in that list still feels unaddressed after reading the linked review, please quote it directly and I'll dig further — happy to keep iterating, just want to avoid re-answering the same list from scratch each round when nothing code-side has changed. CI on the current head was still `in_progress`/`queued` as of my last pass; I'll confirm once it's green. -- 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]
