DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5578537821
@SEZ9 thanks for the careful read, and for agreeing on Blocker 1's tracking split. **On the "cut off" concern:** I checked the stored review body via the API rather than the web renderer, and 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)." followed by the rest of the Blocker 1 writeup through to the "Once Blockers 1 and 2 are addressed..." closing paragraph. So the record is complete as posted; if it displayed cut off, that was a rendering artifact rather than a content gap. Pasting the exact continuation here for the record per your ask, in any case. **On whether this PR's ownership changes depend on the leaked entry being absent:** I traced `finishOwnedResources()`/`finishExecutionContext()` fresh against this head (`18e821c3d`, unchanged from `370f175e879`) to answer this precisely rather than assume it either way. `finishExecutionContext()` (`TaskExecutionService.java:1594-1600`) does `executionContexts.remove(taskGroupLocation, ownedContext)` — a compare-and-remove keyed on the exact context object this tracker owns. That call is safe against *any* current map value: if the map holds something other than `ownedContext` (a leaked entry, a newer generation's context, or nothing at all), the remove simply returns `false` and `finishOwnedResources` takes the "stale" branch (`:1571-1576`), preserving whatever is there. So no, the ownership logic's own correctness does not assume the leaked entry is absent — it was written to tolerate an unexpected map value. What the leak *does* break is upstream of this: if `deployLocalTask` leaks an `executionContexts` entry for a task group whose submit calls threw before any task actually started, no `TaskGroupExecutionTracker` for that generation ever reaches `taskDone()`/`finishOwnedResources()` (the tasks never ran to call it), so that leaked entry is never moved to `finishedExecutionContexts` by this PR's own logic either. And because `deployTask`'s outer guard (`:506`) treats any live `executionContexts` entry as "already active" regardless of whether it's a real running generation or a leaked one, no subsequent redeploy for that `TaskGroupLocation` can even create a new generation to be tracked. So: this PR's `ownedContext`/`finishOwnedResources` path is not incorrect in the presence of the leak, but it is starved — it never gets a chance to run again for that location once the leak occurs, because #12164's bug blocks the very redeploy that would produce the next generation for it to own. Worth noting explicitly, as you said, so the follow-up on #12164 doesn't accidentally get scoped as "the ownership fix's problem" or vice versa — they're independent bugs that compound in exactly this one failure window. **Blocker 2 (stale-generation's own async/timer futures not cancelled):** agreed, still open and still a blocker for this PR specifically — it's this PR's own diff, not baseline-shared with #11757 the way Blocker 1 is. **On the rest (F1/F3, F2, F5, F6, F7, F8):** confirmed still open — the head is genuinely unchanged (`370f175e879` through `18e821c3d` are all empty CI-retrigger commits, no source diff), so nothing has moved since your 08-28/08-31 findings. @davidzollo — to summarize the concrete ask so the next push addresses everything in one pass: please cover Blocker 2/F4 (cancel the old generation's own async/timer-flush futures in the stale-taskDone branch), F1/F3 (either take the deployment monitor for the redeploy-vs-taskDone race on `cancellationFutures`, or fix the javadoc and the race separately — they currently disagree), F2 (`BlockingWorker` should resolve its context from the tracker's `ownedContext`, not the shared `executionContexts` map), and F5 (guard the `put` so it can't silently overwrite a still-live older generation's entry/future). F6-F8 can be legitimate follow-ups if you'd rather keep this push focused — just say so explicitly in the PR description so it's not read as an oversight. Once that lands, I'll do the full from-scratch re-review as usual. -- 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]
