SEZ9 commented on issue #12164: URL: https://github.com/apache/seatunnel/issues/12164#issuecomment-5611672397
Thanks @Rangsh, and nice work turning this around quickly. I have seen the PR link (https://github.com/apache/seatunnel/pull/12218) and will review it as my next step; I have not gone through the diff yet, so I am not making any statement about its contents or CI here. To make that review fast, a few concrete asks on the PR itself, all within the scope we agreed on in this thread: 1. **Rollback boundary**: please call out in the PR description exactly where the new cleanup runs relative to context publication, `onContextPublished`, and task submission, so I can confirm a failure at the callback/submission boundary removes both the `executionContexts` entry and the associated `cancellationFutures` state, and that the pre-publication cleanup path is unchanged. 2. **Regression test determinism**: please note how the post-publication failure is injected (i.e. that it is a deterministic hook rather than timing-based), and confirm the test asserts the second `deployTask` for the same `TaskGroupLocation` actually executes rather than merely returning success. 3. **Master-failover preservation**: please confirm the test for the "already active, healthy task returns success without redeploy" case exercises the existing guard as-is, with no change to its semantics. 4. **Scope**: please confirm the PR touches only `TaskExecutionService` and its test coverage, with no executor or classloader lifecycle changes, and that it is based on current `dev` (I last checked at `f6ed9579c5e`). Nothing further is needed from you on this issue itself; I will move the remaining discussion to the PR once I have reviewed it. <!-- streview-comment:924 --> -- 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]
