SEZ9 commented on PR #12288: URL: https://github.com/apache/seatunnel/pull/12288#issuecomment-5747508037
Thanks for digging into this properly instead of leaving the widened assertion in place. The diagnosis makes sense to me: `checkpointIdCounter.getAndIncrement()` advances inside `createPendingCheckpoint`, so polling the id counter never gave an ordering guarantee relative to checkpoint 1's lifecycle, and waiting on `getLatestCompletedCheckpointId() >= 1` is a much better readiness signal. Restoring the strict `CHECKPOINT_INSIDE_ERROR` assertion is the right call, since that is exactly the regression coverage this test exists for. A few things before I can approve `25693bef`: 1. **Residual window.** The re-review of the current head notes it "still leaves a small residual window." Please spell out concretely what that window is (which interleaving between checkpoint 1 completing and checkpoint 2's barrier dispatch could still produce a different failure reason), and either close it or add a comment in the test explaining why it is acceptable and how narrow it is. 2. **PR title / description.** They still describe the original "accept either reason" approach from `45a8993b`. Please rewrite them to describe the current change: the new test-only getter in `CheckpointCoordinator`, why the old readiness signal was wrong, and that the assertion is unchanged from `dev`. 3. **Production getter.** Please confirm `getLatestCompletedCheckpointId()` is only read from tests and that the assignment at the top of `completePendingCheckpoint` has no effect on any existing path (no locking or ordering change). A one-line Javadoc on the getter stating it is test-only would help. 4. **Commit history.** Since `0ab09e71` did not compile, please squash so that every commit on the branch builds; otherwise bisecting through this range becomes painful. 5. **Flakiness evidence.** Please run the IT in a loop locally (say 50–100 iterations) and paste the result in the PR so we have some data that the new ordering actually holds, not just in reasoning. Once those are addressed I'll take another look. <!-- streview-comment:1191 --> -- 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]
