DanielLeens commented on PR #10878:
URL: https://github.com/apache/seatunnel/pull/10878#issuecomment-4550420442
Thanks for the update. I re-reviewed the latest head against the real
EdgeSocket runtime path, including the current delta after the last
Daniel-reviewed head, and I do not see a reopened source-level blocker on the
current implementation.
## What this PR is trying to solve
- User pain point: edge collectors need a lightweight TCP ingress with
checkpoint-aware commit semantics so they can push batches into a running Zeta
job without guessing whether a batch is really durable yet.
- Fix approach: the current head tightens the protocol path across
`IngressProtocolHandler`, `EdgeSocketSourceReader`, `EdgeSocketSourceState`,
docs, and tests, especially around `DECODE_FAILED`, `DECRYPT_FAILED`, and
restore-time `RESEND`.
- One-line summary: the runtime contract now looks coherent to me; the
remaining gate is that the current GitHub `Build` is still red.
## Runtime path I re-traced
```text
collector connect
-> EdgeSocketSourceReader.open()
-> EdgeSocketIngressServer.start()
-> IngressProtocolHandler.authenticate()
batch ingest
-> "__BATCH__:<batchId>:<payload>"
-> IngressProtocolHandler.dispatchBatch()
-> EdgeSocketSourceReader.handleBatchRecord()
-> deserialize
-> queue offer
-> sourceState.markRecordReceived()
-> RECEIVED / QUEUE_FULL / RETRY / DECODE_FAILED / DECRYPT_FAILED
checkpoint poll
-> "__COMMIT__:<batchId>"
-> IngressProtocolHandler.dispatchCommit()
-> EdgeSocketSourceState.resolveCommitResponse()
-> ACK / PENDING / RETRY / RESEND
restore
-> restoreState()
-> recover watermark
-> set sessionFloorWatermark
-> return RESEND for lost-after-restore batches
```
## Findings
### Issue 1: the current Build is still red
- Location:
- GitHub Actions `Build`
- check-run `77903309129`
- Severity: Medium
- Why this matters:
- I confirmed that the PR-level `Build` check is still failing on the
latest head.
- I also followed the referenced fork workflow (`26458273419`). From the
metadata currently exposed there, I could still see connector IT jobs in
flight, but I did not get a stable failed stack trace that I could map directly
back to a specific source bug in this latest revision.
- Risk:
- even though I do not see a reopened source-level blocker, the head is
still not in a green mergeable state.
- Best fix:
- keep the code surface stable from here;
- rerun / settle the current Build on this head and inspect any persistent
failure with the full log once the workflow output stabilizes.
## Code quality / tests / docs
- The protocol/state split is clearer on the current head than on earlier
revisions.
- Test stability rating for the changed tests: **Stable**.
- `EdgeSocketSourceReaderTest` uses `Awaitility` (`reader::isListening`)
instead of hard sleeps for readiness.
- I did not find shared-global-state or fixed-port anti-patterns that
would make the new tests obviously flaky by design.
- Docs in `docs/en` and `docs/zh` now describe `QUEUE_FULL`,
`DECODE_FAILED`, and `RESEND` consistently with the code path I traced.
## Conclusion
### Conclusion: can merge after fixes
1. Blockers
- Issue 1: the current head still needs a green `Build` result before merge.
2. Suggested follow-ups
- No additional source-level follow-up from this review.
Overall, I do not see a reopened correctness blocker on the latest
EdgeSocket Phase 1 code path. The remaining merge gate is the still-red Build
signal.
--
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]
