DanielLeens commented on PR #10879:
URL: https://github.com/apache/seatunnel/pull/10879#issuecomment-4538714494
Thanks for the large update here. I re-reviewed the latest stacked head from
scratch. The edge-agent direction is clearly moving forward, but the current
revision still inherits one hard blocker from the Phase 1 stack underneath it:
the EdgeSocket checkpoint state format change is not backward compatible with
older snapshots.
# What this PR fixes
- User pain: Phase 2 is trying to assemble a minimal but real Edge Agent
MVP, including WAL, transport, batching, and EdgeSocket ingress.
- Fix approach: the latest head improves raw-bytes transport, WAL resend
behavior, config structure, and multiline collection tests.
- One-line summary: the agent-side direction looks better, but the stacked
head still sits on an unsafe restore-state base from Phase 1.
# Runtime chain I checked
~~~text
Edge agent side
-> WAL / payload / transport improvements
EdgeSocket source side
-> checkpoint / restore still runs through
EdgeSocketSourceState.restoreState()
-> snapshotState() now writes a version byte
-> restoreState() unconditionally reads that byte
-> old checkpoints from the previous head are shifted and misread
~~~
# Findings
Issue 1: the current Phase 2 head still inherits the Phase 1
checkpoint-state compatibility break
- 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: even though this PR adds a lot of agent-side
functionality, the stacked runtime still restores EdgeSocket source state
through the same incompatible old/new snapshot boundary.
- Risk: ACK / RESEND / queued-record recovery can still be wrong after
upgrade, which undermines the correctness of the broader agent stack on top of
it.
- Better fix: resolve the old/new checkpoint compatibility in
EdgeSocketSourceState first, then layer the Phase 2 agent functionality on top
of that safe base.
- Severity: High
# CI note
- The current Build is also red, but Issue 1 is already a standalone
source-level merge blocker even before CI.
# Merge conclusion
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1 must be fixed first. The current stacked head is not safe on the
restore path.
2. Suggested non-blocking follow-up
- Once the Phase 1 state compatibility line is fixed, the newer agent-side
improvements here are worth re-checking on top of that stable base.
Overall, the agent-side work is improving, but the restore-state
compatibility issue underneath it still needs to be closed before merge.
--
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]
