Vivek1106-04 commented on issue #12441: URL: https://github.com/apache/seatunnel/issues/12441#issuecomment-5862580535
@Rangsh @DanielLeens @SEZ9 Recording two bugs here that stay silent on `dev` and that any correct fix for this issue exposes. The agreed plan and constraints above do not mention either of them, and #12493 does not fix them (details in https://github.com/apache/seatunnel/pull/12493#issuecomment-5862502658). I found both while writing my fix and have fixed them in the same change. **1. SubPlan restore race: job stuck in `DOING_SAVEPOINT`** A pipeline that fails while its job is in `DOING_SAVEPOINT` is reset and waits `pipelineRestoreIntervalSeconds` before restarting. `JobMaster.savepointFailed()` learns about the broken savepoint only after that decision and calls `forceStopPipeline()`. That call marks the job as never needing restore, but it finds only reset tasks and stops nothing. The pipeline then restarts after the wait, and the job never leaves `DOING_SAVEPOINT`. On `dev` the lock-held drain hides this, because it delays `cleanPendingCheckpoint` long enough that the timing rarely lines up. Once the drain is moved off the lock, `SavePointTest#testSavePointButJobGoingToFail` fails consistently in the full test run. Any fix for this issue has to include a fix for this race, or it turns `SavePointTest` red. Fix: after the restore wait, `SubPlan` re-checks `jobMaster.isNeedRestore()`. If a stop was decided meanwhile, the pipeline goes back to the terminal state it failed with, keeping its error, so the normal terminal path ends it. Regression: `SavePointTest#testSavePointFailureDuringPipelineRestoreWaitEndsTheJob` with two pipelines. One sink fails the savepoint at 4 s and the other finishes its part at 5 s, so the stop lands inside the 3 s restore wait. It is red on `dev`, where the job stays in `DOING_SAVEPOINT`. **2. Failure reason when the drain is cut short** Constraint 2 above says every interrupt, shutdown, completion and create failure path completes the shared future exceptionally, but it does not say with which reason, and the reason matters. `JobMaster.isSavepointStartPreconditionException` treats a savepoint as never started, and retryable, only for `CHECKPOINT_COORDINATOR_SHUTDOWN` and `TASK_NOT_ALL_READY_WHEN_SAVEPOINT`. `cleanPendingCheckpoint` is called with `CHECKPOINT_COORDINATOR_RESET`, `PIPELINE_END` (from `cancelCheckpoint`), `CHECKPOINT_COORDINATOR_COMPLETED`, or the reason from `handleCoordinatorError`. The natural reading, failing the future with that close reason, is a bug. If a pipeline ends while a savepoint is draining, the job master then handles it as a real savepoint failure even though no savepoint checkpoint was ever created, and `SavePointTest` breaks. A drain ended by `cleanPendingCheckpoint` has to report `CHECKPOINT_COORDINATOR_SHUTDOWN`, whatever the close reason. **Also a correction to the plan:** the gate check belongs next to `pendingCounter > 0` in `tryTriggerPendingCheckpoint()`, not in `createPendingCheckpoint()`, because `createPendingCheckpoint()` cannot re-arm the trigger. I am opening my PR for this issue now, based on `dev`. It contains the drain-off-lock fix with the part 1 regression (red on `dev`), plus the fixes for both bugs above. Part 2 (cross-pipeline) is written and red/green on top of #12165, and will follow once #12165 is merged, so this issue stays open until then, as agreed. -- 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]
