nzw921rx commented on PR #10878: URL: https://github.com/apache/seatunnel/pull/10878#issuecomment-4538861512
> Thanks for the update. I re-reviewed the latest head from scratch, and I found one new blocker on the current revision: the checkpoint state format changed, but the restore path does not stay compatible with snapshots written by the previous revision. > > # What this PR fixes > * User pain: EdgeSocket source needs to restore its batch watermark and queued records correctly after checkpoint / failover. > * Fix approach: the latest head adds a STATE_VERSION byte to the serialized state and expands the state / reader test coverage. > * One-line summary: the new state layout is self-consistent for new snapshots, but it is not compatible with old snapshots from the previously reviewed head. > > # Runtime chain I checked > ``` > Previous snapshot layout > -> lastCommittedBatchId (long) > -> lastReceivedBatchId (long) > -> snapshotWatermark (long) > -> pending / drained / queue data ... > > Current restore layout > -> restoreState() first does in.readByte() > -> then reads committed / received / watermark as longs > > Upgrade path > -> old checkpoints do not have that leading version byte > -> the first long is shifted by one byte > -> restored watermark / batch state becomes corrupted > ``` > > # Findings > Issue 1: the checkpoint state format change is not backward compatible with older snapshots > > * Location: seatunnel-connectors-v2/connector-edge-socket/src/main/java/org/apache/seatunnel/connectors/seatunnel/edgesocket/state/EdgeSocketSourceState.java:158-169,212-217 > * Why this matters: snapshotState() now writes a leading STATE_VERSION byte, but restoreState() unconditionally consumes that byte even for snapshots produced by the previous head, which did not have it. > * Risk: after upgrade, committed / received watermark restoration can be shifted and corrupted, which then poisons ACK / PENDING / RESEND decisions and queued-record restore. > * Better fix: add real old/new format compatibility in restoreState() instead of assuming every checkpoint was written by the new layout. > * Severity: High > > Issue 2: the tests only cover new-format round trips, not legacy snapshot restore > > * Location: seatunnel-connectors-v2/connector-edge-socket/src/test/java/org/apache/seatunnel/connectors/seatunnel/edgesocket/state/EdgeSocketSourceStateTest.java:106-109,125-128,158-159,178-179,196-197 > * Why this matters: the current tests all serialize with the current code and then restore with the current code; they never prove backward compatibility with the previous layout. > * Better fix: add at least one legacy snapshot bytes -> current restoreState() regression test. > * Severity: Medium > > # Merge conclusion > ### Conclusion: can merge after fixes > 1. Blocking items > > * Issue 1 must be fixed first. The current upgrade / restore path is not safe. > > 2. Suggested non-blocking follow-up > > * Issue 2 should land together with the fix so this compatibility line does not regress later. > * The current Build is also red, but Issue 1 is already a standalone merge blocker on its own. > > Overall, the state-versioning direction is understandable, but checkpoint state is a hard compatibility boundary in SeaTunnel and needs old-snapshot handling, not just a new-format round trip. New feature, no legacy compatibility required. -- 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]
