Rangsh commented on PR #12218: URL: https://github.com/apache/seatunnel/pull/12218#issuecomment-5660028013
Thanks @DanielLeens — appreciated the from-scratch re-review on the post-#12238 adaptation. **Issue 1 / Issue 3 (Low):** addressed in `33d20cfe3`: 1. Dropped the redundant second `cancelAsyncFunctionFutures` / `cancelTimerFlushFutures` from `rollbackPublishedDeployment` (`abortAfterFailedPublish` → `cancelAllTask` already covers them). 2. Mirrored `finishExecution` so a rolled-back attempt is recorded into `finishedExecutionContexts` (generation-safe `compute`), and asserted that in the post-publish rollback unit test. **Issue 2 (High / CI):** investigated against run [`34755083145`](https://github.com/Rangsh/seatunnel/actions/runs/34755083145): - `testStaleTaskDoneCleansOnlyOwnedGenerationResources`: confirmed the Mockito `WantedButNotInvoked` on `cancel(false)` after a single `isDone()` interaction. This test is from #12238 and does not call this PR's rollback path. Locally, `TaskExecutionServiceTest` is **20/20 green** (including that method, repeatedly). Hardened the pending-`ScheduledFuture` stub to `doReturn(false).when(future).isDone()` to avoid stubbing-side invocation quirks. A fresh push (`33d20cfe3`) is up so fork CI can re-check determinism on `unit-test` / `engine-v2-it`. - `SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck`: log shows `expected: <CANCELED> but was: <FAILED>` within 1 minute after the intentional worker crash. Will watch the new `engine-v2-it` run; if it reproduces we will dig further (possibly with #12238 owners) rather than wave it away. Will follow up with the isolated re-run results once they finish. -- 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]
