DanielLeens commented on PR #12449: URL: https://github.com/apache/seatunnel/pull/12449#issuecomment-5846323815
Thanks for putting real numbers behind this — a 40/30/8-run stress comparison across `dev`, this PR, #12288, and #12454 is exactly the kind of evidence that should settle a "did we actually fix the race or just get lucky" question like this one. Your reading of the locking matches what I traced independently in my last review: `completePendingCheckpoint` only decrements `pendingCounter` after `notifyCompleted` returns, and `tryTriggerPendingCheckpoint` checks/increments it under the same `lock` the test now borrows, so "`pendingCounter == 0` while holding `lock`" really is a sound "no notification in flight, no new trigger sneaking in" signal rather than a coincidence. 0/78 across your three configurations lines up with the 4/4 clean runs I saw in this head's fork CI, so I'm glad to see it hold up under load and on JDK 8 as well, not just on a quiet machine. On the comparison with #12454: both approaches inject the fault at the same logical point and both came back 0 failures in your run, so I don't see a functional gap between them. This PR's advantage of doing it entirely in test code, with no new hook on `CheckpointCoordinator` itself, is a reasonable tie-breaker if only one is going to land — though which of the two (or whether both add value for different reasons) is ultimately a call for whoever coordinates the overlapping PRs here, not something I want to unilaterally decide in a comment thread. One general note since #12288 came up in your table too: between "close the race so the test deterministically hits the intended path" (this PR's approach) and "widen the assertion to accept either outcome," I'd lean toward the former on principle — it protects the actual regression this test exists for (#10442/#10448), which stops being true if the test can pass having caught the wrong failure. Your numbers for #12288 (still failing 2/40, 5/30, 1/8) seem consistent with that: accepting either reason doesn't remove the underlying nondeterminism, it just stops the test from complaining about it. No change to my conclusion — still Ready to merge / Stable on this PR. Thanks again for the extra rigor here. -- 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]
