SEZ9 commented on issue #12441: URL: https://github.com/apache/seatunnel/issues/12441#issuecomment-5882272417
@Vivek1106-04 thanks for recording these. You're right that neither the agreed constraints nor my PR as posted covers them, and I'll treat both as in scope for the PR rather than as separate follow-ups, since moving the drain off the lock is exactly what exposes them. **1. SubPlan restore race (`DOING_SAVEPOINT`)** Agreed on the analysis: once `cleanPendingCheckpoint` is no longer delayed by the lock-held drain, `forceStopPipeline()` landing during the `pipelineRestoreIntervalSeconds` wait finds only reset tasks and stops nothing, so the restart proceeds and the job never leaves `DOING_SAVEPOINT`. Your fix shape — re-check `jobMaster.isNeedRestore()` after the restore wait and return the pipeline to the terminal state it failed with, keeping its error — is what I'd want in my PR too. I'll add that re-check in `SubPlan` and bring in `SavePointTest#testSavePointFailureDuringPipelineRestoreWaitEndsTheJob` as the regression, confirming it is red on `c3f06f9a82c119e427df0fb1f440bae583148f62` and that `SavePointTest#testSavePointButJobGoingToFail` stays green in the full run. **2. Failure reason when the drain is cut short** Also agreed that constraint 2 is underspecified here. My PR currently fails the shared future with the close reason passed to `cleanPendingCheckpoint` (`CHECKPOINT_COORDINATOR_RESET`, `PIPELINE_END`, `CHECKPOINT_COORDINATOR_COMPLETED`, or the `handleCoordinatorError` reason), which, as you point out, `JobMaster.isSavepointStartPreconditionException` won't classify as never-started/retryable, since it only recognizes `CHECKPOINT_COORDINATOR_SHUTDOWN` and `TASK_NOT_ALL_READY_WHEN_SAVEPOINT`. Could you share the remedy you settled on — which reason the shared future is failed with on each of the interrupt / shutdown / completed / create-failure paths, and whether you widened `isSavepointStartPreconditionException` or mapped the close reasons on the coordinator side? I'd like to match your choice so the two changes don't diverge. If your fix is already in a branch or PR, a link would help so I can either rebase on it or fold your two fixes in with attribution — whichever you prefer. I'd rather not have two competing changes for the same drain. Once I have that, I'll update the PR, redo the red → green table for the new `SavePointTest` case on the same red base, and reply on https://github.com/apache/seatunnel/pull/12493#issuecomment-5862502658. <!-- streview-comment:1383 --> -- 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]
