SEZ9 commented on PR #11677:
URL: https://github.com/apache/seatunnel/pull/11677#issuecomment-5230720838
Thanks for the thorough follow-ups — the latest commits address my concerns
well, and I appreciate that the PR description now matches the traced engine
behavior rather than overstating what the e2e proves.
Confirming where we've landed:
- **Enumerator fix**: `addSplitsBack()` being `synchronized` + re-running
`assignSplits()` when `running == true` is correct defensive hardening at the
contract boundary. The deterministic unit test covers the actual race
independent of engine timing — that's the right primary evidence here.
- **Reachability note**: Agreed with your re-assessment. In the clean
full-pipeline restart the e2e drives (parallelism = 1), `running` is `false` at
`addSplitsBack()` time and the split flows through the pre-existing
`handleSplitRequest()` path; the new branch fires in partial-restart scenarios
where the enumerator survives while a reader restarts. Good that this is now
explicit in the description.
- **Javadoc + `receivedReader()` comment**: The softened `run()` contract
("implementations must not rely on strict invocation order") and the
cross-class invariant comment are both appropriate.
- **e2e improvements**: Replacing the hard sleep with an await on completed
checkpoint count and using the job-state transition as the failure-injection
signal are solid upgrades over the previous log-keyword/sleep approach.
Remaining asks before merge:
1. **e2e stability**: Please run the new e2e a few times locally (or push an
empty commit to re-trigger CI 2–3 times) to confirm the state-based waits don't
flake around the 15s `job.retry.interval` window. Trigger-based failure
injection plus timing-sensitive state assertions is the part I'd most expect to
be flaky under CI load.
2. **Test comment**: Add a short comment in the e2e test itself noting it
exercises the `handleSplitRequest()` restore path and serves as a regression
guard, with the unit test covering the new `running == true` branch — so future
readers don't mistake it for direct coverage of the new code path.
3. Please squash the review-fix commits into a clean history before merge.
The debug-log trace of a real partial-restart interleaving remains a
nice-to-have follow-up, not a blocker. Once (1) and (2) are in, this LGTM.
<!-- streview-comment:103 -->
--
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]
