DanielLeens commented on PR #11819: URL: https://github.com/apache/seatunnel/pull/11819#issuecomment-5627615596
@nzw921rx Thank you for this — I traced all three against the source at this exact head (`74340c7626`) myself rather than taking the write-up at face value, and all three hold up. Since there's no new commit yet, I'm not posting a fresh formal review, but I want to be direct: my prior "Ready to merge" approval on this PR should be treated as superseded by these findings until they're addressed. **Issue 1 (stale split content revalidation) — confirmed.** `MultipleTableFileSourceReader.pollNext` (`MultipleTableFileSourceReader.java:99-116`) only compares `LocalFileIdentity` before and after the read; it never touches `split.getEndContentAnchor()`. I grepped the whole diff for `endContentAnchor` — it's populated on `FileSourceSplit` and used in `equals()`/`hashCode()`, but nowhere in the read path is it actually compared against the file's current state. Since `fileKey` (the identity) survives a `TRUNCATE_EXISTING`-style copy-truncate, your scenario is real: a rewrite that lands strictly between split assignment and split read can pass both identity checks while the bytes underneath have changed, so the reader has no signal to reject the now-stale split. **Issue 2 (global vs. per-file initial/latest classification) — confirmed, and this is a genuine gap in my own approval.** `textTailingInitialScanComplete` (`ContinuousMultipleTableFileSourceSplitEnumerator.java:536-538`) only flips to `true` after a scan completes with zero failures *across every file*, and `enqueueTextTailSplit`'s `initialLatest` check (`:595-598`) reads that same job-wide flag for *any* newly-discovered file, not just the ones present at startup. So a file created after the job starts, while some unrelated file is persistently failing inspection, gets misclassified as "initial latest" and baselined at EOF — its pre-existing rows are silently and permanently skipped. My own hypothesis-testing in the prior review only checked that the baseline is *retained* under partial-scan failure (`testLocalTextTailingLatestRetainsBaselineAfterPartialScanFailure`); it didn't check what happens to a genuinely new file arriving during that same failure window. That's a dist inct case from what I tested, and I missed it. **Issue 3 (header skip on ranged tail splits) — confirmed.** `TextReadStrategy.readProcess` (`TextReadStrategy.java:243-246`) forces `skipHeaderNumber` to `0` whenever `useSplitRead` is true (i.e., any explicit-length tail split), and the enumerator's `discardUntilDelimiter` gate for the first "latest" split has no notion of `skip_header_row_number` at all for a file that's empty or has fewer rows than the configured header count at baseline time. There's genuinely nowhere in `FileTailState` to carry "N header rows still owed" once the baseline is taken before the header exists. All three are correctness bugs on interaction paths my own review round didn't exercise, not restatements of what was already covered. I haven't re-run your "temporary targeted regression tests" locally (no local execution is authorized for this repo this round), but tracing the cited call paths directly against source is enough to corroborate the mechanism you describe in each case — this isn't something I'm taking on faith. Nice catch on all three, especially #2, which is a subtle one. @goutamadwant — given the above, I'd treat these as blockers for merge, not follow-ups. I'll do a full re-review once there's a commit addressing them. -- 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]
