DanielLeens commented on PR #11885: URL: https://github.com/apache/seatunnel/pull/11885#issuecomment-5385704175
Thanks for the deep dive, @SEZ9 — this is a very thorough round. I want to push back on **Issue 1** specifically (the "checkpoint serializer not wired up" blocker), since I went and re-verified it against the actual source before replying rather than taking it at face value. There is no `PendingSplitsStateSerializer` class anywhere in `connector-cdc-base` — I grepped the whole module and found none. Checkpoint (de)serialization for the enumerator state instead goes through the generic path: - `SourceSplitEnumeratorTask.java:112` obtains `enumeratorStateSerializer` from `this.source.getSource().getEnumeratorStateSerializer()`. - `connector-cdc-base` never overrides that method, so it falls back to `SeaTunnelSource.java:120-122`'s default: `return new DefaultSerializer<>();` - `DefaultSerializer.serialize/deserialize` (`seatunnel-api/.../serialization/DefaultSerializer.java`) just does whole-object Java serialization via `SerializationUtils`, on anything that implements `Serializable`. - `IncrementalPhaseState implements PendingSplitsState`, and `PendingSplitsState extends Serializable`. The new `stopOffset` field is typed `Offset`, and `Offset` itself `implements ... Serializable` (`Offset.java:33`). So there's no separate field-by-field serializer to "wire up" here — standard Java serialization walks the whole object graph automatically, and `stopOffset` is captured in the checkpoint bytes with zero additional code. On old checkpoints written before this field existed, default Java deserialization sets the missing field to `null` (which is exactly the behavior the class's explicit `serialVersionUID` and its "Preserve compatibility with checkpoints written when this state had no fields" comment are guarding), and `IncrementalSplitAssigner`'s restore path already treats `null` as "not yet resolved" and falls back correctly. This matches what I'd already confirmed in my round-3 review (section 3.4): "checkpoint compatibility for `IncrementalPhaseState`'s new `stopOffset` field is correctly handled." So I don't think Issue 1 holds as a blocker — happy to be shown otherwise if I've missed a serializer somewhere, but I checked this concretely rather than by inspection alone. I haven't independently re-verified Issues 2–8 to the same depth yet (this is a lighter-weight follow-up pass, not a full re-review round), but Issue 5 (unguarded `offsetFactory.latest()` DB call with no retry on the enumerator's completion path) and Issue 4 (docs not updated for the `stop.mode=latest` behavior change) both look like reasonable, worth-addressing points on a quick read. Separately, please note this PR still has an open blocker from my own round 3 that's independent of anything here: `HybridSplitAssignerTest.testCompletedSnapshotPhase` NPEs deterministically against `completedSnapshotPhase()`'s new code due to a missing null-guard on `context.getSourceConfig()` — that's currently the thing actually keeping CI red on this head, and it'll need a fix alongside whatever comes out of this round's discussion. -- 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]
