SEZ9 commented on PR #12623: URL: https://github.com/apache/seatunnel/pull/12623#issuecomment-6029430395
@Vivek1106-04 thanks for the detailed follow-up. **F1 (merge order).** Agreed. With the dependency on #12511 stated in the description and the "independent" wording removed, I'm fine not duplicating the post-sleep `isNeedRestore()` re-check here, since whichever PR lands second would otherwise have to remove it. This PR should stay dependent on #12511 going in first; once it does, please rebase so the `abandonRestore` path and your `forceStop()` change are exercised together on the same base. **F2 (pipeline-level test).** Your reasoning for pinning the contract at the vertex level makes sense to me: after #12511, a `forceStopPipeline()`-during-restore-wait test would pass with or without this fix, so the vertex tests in f24ed531e (`testForceStopCompletesTheFutureOfATaskResetForRestore`, `testForceStopLeavesTheStateProcessStoppedWhenTheTransitionIsSkipped`, and the `isRunning` assertions on both force-stop tests) are the right place to guard it. I'll go through those tests with the fix reverted and cross-check the pipeline-level assertions you linked in #12511 before marking F2 resolved, so it stays open until then. Remaining from my side: #12511 landing, a rebase on top of it, and my final pass over the tests. <!-- streview-comment:1565 --> -- 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]
