SEZ9 commented on issue #12339: URL: https://github.com/apache/seatunnel/issues/12339#issuecomment-5806284362
@Vivek1106-04 You're right, the tail was cut off on my side — apologies for the extra round trip. Answering the one-line question directly: the split stands. The savepoint-lock coupling is a tracked follow-up, not a gate on #12165. I am not asking you to add the regression to #12165; it belongs with #12441. Concretely, for #12165: 1. Keep the scope as you described: the watchdog half of the coupling via `da0cbbf52` (`CheckpointCoordinator#expireCheckpoint`, covered by `CheckpointCoordinatorTest#testCheckpointExpiryHandlingNeverRunsOnTheDispatchThread`, verified red with the fix reverted). No new test commits needed on the approved head `c773cc091` for the savepoint case. 2. The Correction section and the `SharedCheckpointScheduler` class Javadoc must state plainly that the PR does not remove the pre-existing savepoint lock coupling and link #12441 as the owner of moving the wait off the lock — your proposed `startSavepoint` bullet does that. 3. The Test Plan bullet you drafted is the right wording: the regression that fills dispatch capacity with savepoint-contended coordinators and asserts an unrelated pipeline still receives its trigger and watchdog work lands with #12441, written red against the current blocking lock acquisition first and then made green by the fix. Please carry that exact red-first requirement into #12441 so it is not lost. One thing to be clear about: agreeing the split is not an acceptance of #12165. The current head `c773cc091` still shows a failed Build check, so it is not mergeable as it stands. Once that is resolved or explicitly attributed (flake, infra, unrelated), I will do the final pass on the documented scope only. Your bounded-not-unbounded point on the coupling (needs trigger and savepoint windows to coincide) is a fair characterization and fine to include in the Correction text as stated, given you have not been able to demonstrate full pool occupancy through it. Scheduling-delay observability stays ahead of any thread-count or tuning option, as before — no change there. Remaining asks: - Confirm the Correction/Javadoc/Test Plan wording above is in the PR text, or push it if it is not yet. - Resolve or attribute the failed Build check on `c773cc091`. - Open or update #12441 with the red-first regression requirement so the handoff is recorded there, not only in this thread. <!-- streview-comment:1265 --> -- 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]
