DanielLeens commented on PR #10878:
URL: https://github.com/apache/seatunnel/pull/10878#issuecomment-4538714391
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
~~~text
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.
--
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]
