li3zhi4 commented on PR #11885: URL: https://github.com/apache/seatunnel/pull/11885#issuecomment-5390180456
Thanks to both @DanielLeens and @SEZ9 for the thorough round-3/round-4 reviews. All findings are now addressed on head `a09045249`; summary per issue: **DanielLeens round-3 Issue 1 (NPE in `completedSnapshotPhase`, High)** — fixed. The `stop.mode=latest` resolution no longer lives in `completedSnapshotPhase()` at all (it moved into `createIncrementalSplit()`, see below), so the method no longer dereferences `context.getSourceConfig()`; `HybridSplitAssignerTest.testCompletedSnapshotPhase` additionally now supplies a mocked `SourceConfig` (`StopMode.NEVER`) to keep its intent explicit, matching the existing pattern in `IncrementalSplitAssignerTest`. **SEZ9 Issue 2 / DanielLeens confirmation (single authoritative resolution point)** — fixed, and it also subsumes the NPE fix above. `createIncrementalSplit()` now resolves `latest()` exactly once (guarded by `resolvedStopOffset == null`) and reuses the same value for every subsequent split and for the checkpoint (`snapshotState`). The split handed to the reader and the checkpointed value can no longer diverge, so a restore cannot silently move the stop boundary — and the ordering concern you both traced (split created before `completedSnapshotPhase` runs) is gone because there is only one resolution point. Unit test updated to assert resolve-once (`times(1)`) and reuse-after-restore. **SEZ9 Issue 1 / Issue 6 (checkpoint serialization)** — as @DanielLeens verified, `connector-cdc-base` has no hand-written `PendingSplitsStateSerializer`; enumerator state persists via plain Java serialization (`PendingSplitsState extends Serializable`), with `IncrementalPhaseState.serialVersionUID` deliberately unchanged so legacy checkpoints deserialize with `stopOffset = null` and fall through to the pre-existing resolution path. To close Issue 6 empirically rather than by argument, I added `IncrementalPhaseStateTest`: (a) a real `ObjectOutputStream`/`ObjectInputStream` round-trip preserves `stopOffset`; (b) a legacy single-arg state deserializes with `stopOffset == null`. **SEZ9 Issue 3 (readiness gate not structural)** — addressed. Two changes: (1) the `initial` case now bulk-inserts 2000 rows (with `snapshot.split.size = 20` → 100 splits, parallelism=1, ascending-key order), so the sink-visibility gate (first row visible) can only fire while ~99 splits are still unread — the UPDATE is guaranteed to land inside the snapshot window without relying on a timing margin; (2) a defensive assertion right after the gate fails loudly with "snapshot finished too early: last-chunk row already in sink" if the whole snapshot ever completed before the gate fired, instead of letting the post-UPDATE assertion mislead as data loss. Note on the suggested row-lock stall: it is not usable here, because the snapshot reader issues a plain MVCC `SELECT * FROM ... WHERE id <= ?` (no `FOR SHARE`/`FOR UPDATE`), so an open `FOR UPDATE` transaction on a trailing row does not block it — I verified the generated SQL directly. **SEZ9 Issue 4 (docs)** — done. `stop.mode = "latest"` snapshot-window semantics (stop offset resolved when the snapshot phase completes; changes written while the snapshot runs are not dropped) documented in `MySQL-CDC.md` (en/zh), in both the option table and a new Bounded-Read example; the PR checklist's docs item is now checked. **SEZ9 Issue 5 (no exception handling on `latest()`)** — fixed. The resolution point retries with a short backoff (3 attempts, mirroring the CDC connection-factory retry pattern), so a transient DB failure at the snapshot-to-incremental transition no longer fails the whole job. **SEZ9 Issue 7 / Issue 8 (PR description alignment)** — done. The PR description now describes the enumerator-side mechanism (`createIncrementalSplit()` / `IncrementalPhaseState.stopOffset`) and notes the fix applies to all `connector-cdc-base` connectors, not MySQL specifically; no reader-side attribution remains. **Verification (local, head `a09045249`)**: `spotless:check` ✅; `connector-cdc-base` unit tests 71/71 ✅ (incl. new `IncrementalPhaseStateTest`); e2e 7/7 ✅ (initial+earliest 2/2, latest+specific+timestamp 3/3, existing specific-stop regression 2/2); `git diff --check` clean. The previously-failing `unit-test (11, ubuntu-latest)` NPE is fixed and no longer blocks CI. Happy to take another pass from either of you. -- 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]
