SEZ9 commented on PR #11487: URL: https://github.com/apache/seatunnel/pull/11487#issuecomment-5077822723
Thanks @fatmanverse for the thorough follow-ups — I've gone through `e22ab12d7` and `aa2705213` against my earlier review notes, and all three of my medium concerns are now addressed: 1. **Restore edge case** — Making the checkpointed table set authoritative on the snapshot-only restore path (and marking legacy state as having checkpointed remaining tables before the assigner opens) is the right call. The unit test asserting `discoverDataCollections()` is never invoked on recovery is exactly the regression coverage I wanted. 2. **`exactly_once` contract** — Good, keeping both `initial` and `snapshot` supported and documenting the bounded low-to-high-watermark backfill semantics resolves the doc-narrowing concern. 3. **`server-id` range** — The `5657-5658` range with parallelism 2 in the E2E fixes the example inconsistency, and the concurrent-write test design (updates/deletes against completed snapshot ranges while later splits are outstanding, sink stays equal to the original snapshot) is a solid way to prove the bounded semantics. Also appreciated: - Reusing `MysqlCDCDriverResolver` instead of downloading the driver inside the container — that keeps the E2E hermetic. - Fail-fast validation now running before datasource/dialect/offset init, with independent negative coverage for `stop.mode`, `startup.timestamp`, and both `startup.specific-offset.*` keys. - Retaining both existing public `HybridSplitAssigner` constructors preserves binary compatibility for other dialects. On the environment notes: - The `-Dtestcontainer.version=1.21.4` override for Docker 29/OrbStack is fine as a local workaround since no POM change is included — just confirming CI runs with the repo default, so we'll rely on the CI result there. - The `connector-jdbc` compilation failures and the `seatunnel-config-shade` issue on `origin/dev` are indeed pre-existing and unrelated to this PR; no action needed from you on those here. Remaining asks before merge: 1. Please rebase onto the latest `dev` once more and make sure the full CI pipeline (including the CDC E2E jobs) goes green — that's the last gate. 2. Double-check the PR title/description reflect the final scope so the changelog entry is accurate (it sounds like you've already updated it — just flagging for a final look). Once CI is green I'm happy to approve. Nice work on this one — the feature is well-scoped and the test coverage came out strong. <!-- streview-comment:5 --> -- 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]
