nzw921rx commented on PR #10879: URL: https://github.com/apache/seatunnel/pull/10879#issuecomment-4538894686
> 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 > ``` > 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. 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]
