DanielLeens commented on PR #12288: URL: https://github.com/apache/seatunnel/pull/12288#issuecomment-5793378246
Thanks for the careful follow-up, @SEZ9 — and thanks for the +1 on the `45a8993b` point, that matches what I found too. Since there's no new commit on top of `25693bef` yet, I'm answering your five points directly here rather than posting a whole new review against the same code. **1. Residual window.** Here's the concrete interleaving. `getLatestCompletedCheckpointId()` is backed by `latestCompletedCheckpoint`, which is assigned at the very top of `completePendingCheckpoint` (`CheckpointCoordinator.java:1386`) — strictly *before* that same call invokes `notifyCompleted(...)` at `:1391` and *before* it decrements `pendingCounter` at `:1398`. So there are actually two distinct windows, not one: - (a) The test's 50 ms poll can observe checkpoint 1's id the instant it's written at `:1386`, and the test thread can remove the `ownedSlotProfilesIMap` entry before checkpoint 1's *own* `notifyCompleted` call (still in flight at `:1391`) finishes its `queryTaskGroupAddress` lookup. If that happens, checkpoint 1's own notify hits the missing entry and you'd get `CHECKPOINT_NOTIFY_COMPLETE_FAILED` again — for checkpoint 1 this time, not checkpoint 2. This window is bounded by the gap between a volatile field write and an RPC dispatch a few lines later — realistically sub-millisecond to low-single-digit-milliseconds — against a 50 ms poll, so it should be rare in practice. - (b) Checkpoint 2 creation is gated by `pendingCounter.get() > 0` (`:820-825`), and the counter isn't incremented until checkpoint 2 is actually created (`:1024`). If checkpoint 1 itself runs long on a loaded CI box (close to or past the 2000 ms `checkpoint.interval`, unchanged by this PR), the next trigger attempt can land very close to checkpoint 1's completion, and the pending-checkpoint retry granularity is 500 ms (`:821-822`) — so checkpoint 2 could in principle get created and dispatch its own barrier before the test thread finishes the removal. Both windows still fail *closed*: whichever call site observes the missing map entry, it still goes through `handleCoordinatorError` and fails the job with *some* `CheckpointCloseReason`, so the strict, unchanged assertion (`CheckpointCoordinatorFailoverIT.java:857-865`) would simply flake red rather than silently pass — it can't mask a real regression in #10448's fix, only produce an occasional false failure. I'd lean toward your suggestion of adding a `pendingCounter`-based signal (wait for `getLatestCompletedCheckpointId() >= 1 && pendingCounter == 0`, which really does mean "notify finished") to close window (a), plus bumping `checkpoint.interval` a bit to widen the margin on (b) — happy to see that as a follow-up if you'd rather not block on it, since it's strictly safer than `dev` today either way. **2. PR title / description.** Confirmed still stale — I just re-pulled the live PR and the title/body are unchanged from `45a8993b`'s "accept either reason" framing, including the "no production code touched" line, which is no longer accurate now that `CheckpointCoordinator.getLatestCompletedCheckpointId()` exists. This needs a rewrite before merge; I'd suggest something like "Make the trigger-dispatch fault injection in CheckpointCoordinatorFailoverIT deterministic" for the title, and a body that describes the counter-vs-completion root cause, the new accessor, and that the assertion is unchanged from `dev`. **3. Production getter confirmation.** Did the check you asked for: a repo-wide search for `getLatestCompletedCheckpointId` turns up exactly one caller outside its own declaration — `CheckpointCoordinatorFailoverIT.java:974` — nothing in any production or connector module. And diffing `CheckpointCoordinator.java` against `dev` shows the change is a pure addition: the pre-existing assignment, the `notifyCompleted` call, and the `pendingCounter` decrement inside `completePendingCheckpoint` are byte-for-byte untouched, so the new getter can't have reordered or added a side effect to that path — it's a plain read of an already-existing volatile field. The Javadoc already says it "exists purely for test observability," matching the existing `getReadyToCloseImapKey()` idiom in the same class, so I think this one's answered. **4. Commit history.** Agreed, and confirmed: `0ab09e71`'s own follow-up commit message says outright that it didn't compile (an effectively-final lambda capture that `25693bef` fixes). That's a real bisect trap. Squashing before merge makes sense here. **5. Flakiness evidence.** I want to be upfront rather than hand-wave this: as an AI reviewer working in a shared read-only checkout, I'm not able to run the IT locally in a loop — this repo's local-verification policy limits me to `spotless:apply`/`spotless:check`, with all functional/runtime verification coming from the PR's own CI. What I can point to is that the fork CI run on `25693bef` did execute `CheckpointCoordinatorFailoverIT` end-to-end on both JDK 8 and JDK 11 and reported `Tests run: 4, Failures: 0, Errors: 0` on both — but that's one pass per JDK, not the 50-100x signal you're asking for. Getting that will need a human to actually run the loop locally; I can't manufacture it. Net: nothing here changes the underlying conclusion from my last full review — still **Ready to merge after fixes** — but points 1 and 2 are real, unresolved action items, and 4 is worth doing before the final merge. Happy to take another look as soon as the title/description are updated and the branch is squashed. -- 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]
