Vivek1106-04 commented on PR #12493: URL: https://github.com/apache/seatunnel/pull/12493#issuecomment-5862502658
@Rangsh thanks for the PR. Moving the drain out of the lock exposes two bugs that stay silent on `dev`, and this PR does not fix either of them. **1. SubPlan restore race (job stuck in `DOING_SAVEPOINT`)** If the job master decides to stop the job while a failed pipeline is in its restore wait, `SubPlan` still restarts that pipeline after the wait. The job then never leaves `DOING_SAVEPOINT`. This bug already exists on `dev`, but the lock-held drain hides it because it delays `cleanPendingCheckpoint`. With any correct fix for this issue, `SavePointTest#testSavePointButJobGoingToFail` fails consistently in the full test run. This PR does not change `SubPlan`, and the PR only reports the 4 new `CheckpointCoordinatorTest` tests, not `SavePointTest`. **2. Wrong failure reason when the drain is cut short** `JobMaster.isSavepointStartPreconditionException` treats a savepoint as never started, and retryable, only for `CHECKPOINT_COORDINATOR_SHUTDOWN` and `TASK_NOT_ALL_READY_WHEN_SAVEPOINT`. In this PR, `cleanPendingCheckpoint` maps only `CHECKPOINT_COORDINATOR_RESET` to `SHUTDOWN` and passes every other `closedReason` through unchanged: `PIPELINE_END` from `cancelCheckpoint`, `CHECKPOINT_COORDINATOR_COMPLETED`, and the reason from `handleCoordinatorError`. If a pipeline ends while a savepoint is draining, the shared future fails with a reason that is not retryable. The job master then handles it as a real savepoint failure, even though no savepoint checkpoint was ever created. A drain that is cut short has to report `CHECKPOINT_COORDINATOR_SHUTDOWN`. I already have both fixes, with regression tests, together with the drain-off-lock change for this issue. As I said on #12165 (https://github.com/apache/seatunnel/pull/12165#issuecomment-5832680424), the work is done and is waiting for #12165 to merge, because part 2 (cross-pipeline) can only go red on the shared dispatch pool. I will open that PR as soon as #12165 lands. -- 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]
