li3zhi4 commented on PR #11885: URL: https://github.com/apache/seatunnel/pull/11885#issuecomment-5351017608
Thanks for the detailed and fair review, @DanielLeens. I've reworked the fix to **Option B** (resolve once at the enumerator when the snapshot phase completes), addressing both High issues: **Issue 1 (restart/checkpoint stability) — fixed via Option B:** - `IncrementalSplitAssigner.completedSnapshotPhase()` now resolves `stop.mode=latest`'s stop offset **exactly once**, when the snapshot phase is confirmed complete, and stores it in `IncrementalPhaseState` (new `stopOffset` field, checkpoint-compatible). - `createIncrementalSplit()` prefers the resolved stop offset; `specific`/`timestamp`/`never` keep `StopConfig.getStopOffset()` (unchanged behavior). - The reader-side re-resolution in `MySqlBinlogFetchTask.execute()` is **reverted** — no per-task `SHOW MASTER STATUS` anymore, so a restart reuses the checkpointed value and cannot drift. - New unit test `IncrementalSplitAssignerTest.shouldResolveLatestStopOffsetOnceAtSnapshotCompletionAndReuseAfterRestore` asserts: resolved once at snapshot completion (`verify(offsetFactory, times(2)).latest()`), and a restored assigner reuses the same offset (no re-resolution after restore). **Issue 2 (CI-flaky E2E) — root-caused and fixed:** - The failure was a readiness race: with a single-row table, the snapshot phase can complete before the `RUNNING` readiness signal is observed, so the post-readiness `UPDATE` landed after the (snapshot-completion-time) stop offset and was silently dropped. - The test now bulk-inserts 200 rows before starting the job, making the snapshot phase of `initial`/`earliest` startups measurably non-trivial, so the `UPDATE` issued after `RUNNING` is guaranteed to land inside the snapshot window and be captured by the binlog phase. Verified locally: `testMysqlCdcInitialStartupWithLatestStop` + `testMysqlCdcEarliestStartupWithLatestStop` pass (2/2), and the full 7-test matrix (5 latest-stop combos + 2 existing specific-stop tests) passes. **Issue 3 (unit coverage) — covered** by the new `IncrementalSplitAssignerTest` above. Local verification for this head (`c58fc10c3`): `spotless:check` ✅, compile ✅, unit tests ✅, E2E 7/7 ✅. CI is re-running on the fork now — happy to address anything that comes up. -- 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]
