zhangshenghang commented on PR #11131: URL: https://github.com/apache/seatunnel/pull/11131#issuecomment-4763661284
Nice cleanup. The round-robin striping plus `LinkedHashSet` reads much clearer than the old contiguous-block math, and switching the loop counter to `long` is a welcome side benefit — the old `int readerStart = i * splitRowNum` could silently overflow on very large tables, and the new splits are cleanly aligned to `split_row` instead of being clipped at reader boundaries. Two things I'd like to see addressed before merge. **1. The tests don't actually cover the thing this PR fixes.** The whole point of the change is preserving order, but `MaxcomputeSourceSplitTest` only asserts how many splits land on each reader — nothing checks that splits arrive in ascending `rowStart` per reader, which is the behavior the `LinkedHashSet` swap is there to guarantee. A future refactor could quietly reintroduce a `HashSet` and these tests would stay green. The tests also re-implement the `discoverySplits` loop inline rather than driving the enumerator, so they validate a copy of the logic, not the production path. At minimum, could you add an assertion that the splits collected per reader are in ascending `rowStart` order? That's the regression guard that matches the PR's intent. **2. Resuming an in-flight job from an older checkpoint will re-read data.** `discoverySplits()` dedups against restored state with `assignedSplits.forEach(splits::remove)`, which relies on split equality (all fields, including `rowStart` and `index`). This PR changes both the `rowStart` boundaries and the `index` assignment scheme, so a job checkpointed on a pre-change build and resumed on this one won't match any restored split — every split gets regenerated and reassigned, on top of the splits each reader restores from its own state, so previously-consumed rows get read again. Within a single build it stays deterministic and restore is fine, so this is strictly an upgrade-time concern. But since the PR description says "no user-facing change," it'd be worth at least calling out that resuming an interrupted job across this upgrade re-reads data. Not necessarily a blocker, just shouldn't be silent. -- 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]
