SEZ9 commented on PR #12567: URL: https://github.com/apache/seatunnel/pull/12567#issuecomment-5965041640
Thanks for the fix and the clear write-up. I agree the fetcher filter is the right and minimal place for this change, so nothing changes about the direction. Three points from my earlier review are still open before I can approve: 1. **Per-table fallback watermark.** The fallback is the split's startup offset, which is the *minimum* of the restored table watermarks. For the single-table example in the write-up that equals `t1`'s own checkpoint offset, so the example works. With incremental parallelism > 1, though, a restored table whose own checkpoint offset is higher than the split minimum would have events between the two offsets treated as not yet delivered and re-emitted under exactly-once. Could you either use each restored table's own watermark from the assigner's `tableWatermarks` (carrying it into the split so `configureFilter()` can `putIfAbsent` the per-table value), or explain why the per-table offsets are guaranteed to be equal at restore time? If the latter, a short code comment would help future readers. 2. **IT for the fixed path.** The existing restore-with-new-table e2e runs without exactly-once and never writes to the pre-existing table after restore, so it would have passed before this change too. Please extend it (or add a variant) that enables exactly-once, restores with the added table, then mutates the pre-existing table and asserts those rows arrive at the sink. That is the regression this PR is meant to prevent. 3. **Reflection in the unit test.** Invoking the private filter method via reflection couples the test to a private name. Please either drive the fetcher through `submitTask` so `configureFilter()` runs on the normal path, or expose the hook through a test-visibility annotation instead of reflection. Once 1 is settled and 2/3 are in, I'm happy to take another look. <!-- streview-comment:1485 --> -- 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]
