SEZ9 commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5595102457
@DanielLeens thanks for the consolidated re-review on `eb7beaa288f`. Since your diff of `39312b92d..eb7beaa288f` shows no file changes, there is nothing new to evaluate. Your write-up of the problem (a stale `TaskGroupExecutionTracker` from a previous restore generation tearing down a newer generation's resources via location-only keys) matches my understanding, and binding the tracker to `ownedContext` with an identity check under the `TaskExecutionService.this` monitor is the right direction. The previously raised points still apply to the current head, though: 1. **F1 / F4 – work under the service-wide monitor.** `finishOwnedResources` cancels futures and releases the classloader while holding the same lock `deployLocalTask` uses to publish a context. Cancelling `CompletableFuture`s runs dependent callbacks synchronously on the cancelling thread, so this blocks every deploy on the node and is a latent deadlock vector. Please do only the identity check and map mutation under the lock, and perform the actual cancellation/classloader release after leaving the monitor. 2. **F2 – stale branch leaks.** When the tracker detects it is stale it returns without touching `taskAsyncFunctionFuture` / `timerFlushFutures`, but those maps carry no generation identity, so the old generation's entries are never cancelled. Either tag those entries by context (or tracker) so the stale branch can clean up only its own, or capture the old generation's futures at deploy time and cancel them from the tracker. 3. **F3 – rollback after `cancellationFutures.put`.** The `put` now happens before `onContextPublished.run()`; if the hook (or a later submit) throws, the existing rollback path leaves a live `cancellationFutures` entry. Please extend the rollback to remove it, using the same `remove(key, value)` identity semantics. 4. **F6 – unconditional overwrite in `deployLocalTask`.** Overwriting `cancellationFutures` for the location orphans a possibly-live future from the previous generation. Please either complete/cancel the displaced future explicitly or assert it is already done. 5. **F5 – test hygiene.** The new test injects entries into the shared `TaskExecutionService` maps and never removes them; please clean them up after the test so later tests on the shared server aren't affected. 6. **F7 / F8 – invariant and logging.** The `get()` + `remove(key, value)` double check in `finishExecutionContext` relies on every `executionContexts` writer holding the service monitor, which isn't documented or enforced. Please add a short comment stating that invariant (ideally with an assertion), and adjust the stale-branch warning so it doesn't claim a newer generation exists when the entry is simply absent. Once these land in a commit with actual changes, I'll do a delta review promptly. <!-- streview-comment:919 --> -- 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]
