SEZ9 commented on PR #12623: URL: https://github.com/apache/seatunnel/pull/12623#issuecomment-6487759475
Thanks @Vivek1106-04 for the quick sync. Merging `dev` (`f7a575432`) into the branch so the head is now `caa5f098f`, keeping dev's imports in `StateTransitionCleanupTest` with no test body change, and `PhysicalVertex` merging cleanly all sound right. The local JDK 17 results for `org.apache.seatunnel.engine.server.dag.physical.**` (30/30, `spotless:check` clean) are good to see; please do post the outcome of the fork Build (run `40727927696`) once it finishes. Since you confirmed `forceStop()` and the test bodies are unchanged by the merge, the two previously raised points still stand on `caa5f098f`: 1. **Restore thread not stopped by completed futures (PR12623-F1, `PhysicalVertex.java`).** Completing the task futures while `SubPlan.prepareRestorePipeline()` is sleeping does not stop the restore thread on this base: it still releases/re-applies slots and restarts the state process for a pipeline whose tasks are already `CANCELED`, because there is no post-sleep `isNeedRestore()`/end-state re-check. Could you either add that re-check after the sleep (and before slot re-application / state process restart), or explain why the restore thread's subsequent work is harmless in that state? If you believe this is covered by what lands via #12511, please point to where, so I can verify the merge order actually makes it safe. 2. **Test does not exercise the target scenario (PR12623-F2, `StateTransitionCleanupTest.java`).** The current test drives a bare vertex in `CREATED` with no `SubPlan` or `JobMaster`. It would be good to have a test that reproduces the #12622 path: a pipeline waiting in restore, `forceStopPipeline()` invoked, and the job leaving `DOING_SAVEPOINT`. Even a `SubPlan`-level test that asserts the pipeline future completes after `forceStopPipeline()` on tasks whose state process is not running would give us end-to-end confidence that the fix does what the description says. Once those two are addressed (or argued away) and the fork Build result is in, I will take another pass. Agreed on the landing order after #12511. <!-- streview-comment:2124 --> -- 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]
