DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5578784666
Pushed a fix at `e66911c0c` (maintainer-edit, as davidzollo). Going through your asks one by one: **Truncation claim**: checked the stored review body via the API end-to-end - it is not actually truncated. The sentence after "either submit call" reads in full: "(e.g. `RejectedExecutionException` from an executor mid-shutdown, exactly the kind of race a failover/restore window produces) leaves the `executionContexts`/`cancellationFutures` entries in place with no rollback (this is SEZ9's F3)." Whatever you saw was likely a rendering artifact on GitHub's side, not a data loss on ours - happy to paste any other section if something still looks cut off to you. **Does the ownership fix depend on Blocker 1's leak being absent?** No. `finishExecutionContext`'s `executionContexts.remove(taskGroupLocation, ownedContext)` is a pure reference-identity compare-and-remove; it doesn't care whether some *other*, unrelated deployment attempt elsewhere leaked an entry. And if Blocker 1's leak *did* occur for this exact location, `deployTask`'s `containsKey` guard would already be permanently blocking any new deployment there (that's the whole point of Blocker 1's severity) - so no new generation would ever reach `asyncExecuteFunction`/`registerTimerFlushTask` to register anything that could conflict with this fix's tagging. The two are orthogonal. **Blocker 2/F4 (fixed)**: `taskAsyncFunctionFuture`/`timerFlushFutures` entries are now tagged with the `TaskGroupContext` active at registration time (`OwnedFuture`, `TaskExecutionService.java`). `finishOwnedResources`'s stale branch now calls `cancelOwnedAsyncFunctionsInPlace`/`cancelOwnedTimerFlushTasksInPlace`, which cancel and remove only entries tagged with `ownedContext`, leaving anything tagged with a different (necessarily newer) context strictly untouched. `registerTimerFlushTask`'s existing exact-`TaskLocation` replace-and-cancel logic didn't need the tag (re-registering for the same `TaskLocation` is always a legitimate replacement, regardless of generation) - only wrapped for the type change. **F1/F3 (redeploy-vs-taskDone race on `cancellationFutures`)**: I checked reachability before deciding how to fix this - grepped the whole repo for callers of the unguarded public `deployLocalTask()` overload; the only caller anywhere is `TaskExecutionServiceTest`. Production RPC always goes through `deployTask()`, whose `synchronized(this)` block wraps the entire `containsKey` check + `deployLocalTask()` call, so the race can't fire in production today. Given that, I took your documentation option: both `deployLocalTask()`'s and `finishOwnedResources()`'s javadocs now state the actual contract precisely (the guard lives in `deployTask`'s caller-side block, not internally), rather than introducing new runtime guard logic whose semantics I'd have to invent for a path nothing production calls. **F5 (plain put overwrites a live entry)**: same root cause and same fix as F1/F3 - it's the `deployLocalTask()` overload's lack of self-guarding, now documented as such. **F2**: already fixed at this head - `BlockingWorker.run()` resolves via `taskGroupExecutionTracker.ownedContext` (not the shared map), with a comment explaining why. I think this is stale from an earlier round; let me know if you're seeing a different call path still using the shared map. **F6/F7/F8**: F6 remains structurally neutralized by `deployTask`'s `containsKey` guard, per my last assessment. F7 (test coverage) - added `testStaleTaskDoneCancelsItsOwnAsyncAndTimerFutures`, specifically covering the redeploy-vs-taskDone commingled-bucket scenario (old generation's own async/timer entries alongside a replacement generation's, verifying only the stale ones get cancelled). F8 (dead-code branch, `finishExecutionContext` doc gap) still stands as a documentation-only nit - happy to fold in as a follow-up if you'd like it in this PR too, just say so. Verified locally (compile + full `TaskExecutionServiceTest` suite, 18/18 passing, including both stale-generation tests) before pushing, given how central this class is. -- 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]
