Vivek1106-04 commented on issue #12339: URL: https://github.com/apache/seatunnel/issues/12339#issuecomment-5789146514
@SEZ9 The comment is complete on GitHub; it ends with "Tell me which and I will start.", so I think the tail was lost on your side at a line wrap. The question, in one line: > DanielLeens accepted the savepoint lock-holding as a tracked follow-up on #12165, while you kept the savepoint-contention regression as a gate on acceptance; which reading stands? Since then @DanielLeens has approved #12165 on `c773cc091` ([review](https://github.com/apache/seatunnel/pull/12165#pullrequestreview-5285689696)). His position there: the savepoint-lock case is fine as a tracked follow-up, and the coupling is "intentionally left as a documented, tracked gap rather than something this PR's own test suite needs to reproduce." So I am proposing the split you offered to evaluate, with both of your conditions met: 1. **The Correction section links the follow-up.** The `startSavepoint` bullet names #12441 as where moving the wait off the lock is tracked. 2. **The Test Plan states where the regression lands.** New bullet: the regression that fills dispatch capacity with savepoint-contended coordinators and asserts an unrelated pipeline still receives its trigger and watchdog work lands with the #12441 fix, written red against the current blocking lock acquisition first and then made green by the fix. What #12165 does and does not claim, so the split is easy to judge: - It fixes the watchdog half of the coupling (`da0cbbf52`: `CheckpointCoordinator#expireCheckpoint`, covered by `CheckpointCoordinatorTest#testCheckpointExpiryHandlingNeverRunsOnTheDispatchThread`, verified red with the fix reverted). - It does not fix the savepoint half, and says so in the `SharedCheckpointScheduler` class Javadoc and in the STIP Correction, both pointing at #12441. - The coupling is bounded by the length of a savepoint, not unbounded, and reaching simultaneous occupancy of the whole dispatch pool through it needs trigger and savepoint windows to coincide, which I have not been able to demonstrate. If you still want the regression in #12165 rather than with #12441, say so and I will write it there, red first, with the non-blocking lock acquisition as the fix. That means new commits on an approved PR and another review round, which is why I am asking rather than doing it. Scheduling-delay observability stays ahead of any thread-count or tuning option, as before. -- 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]
