SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5578009916
Thanks @DanielLeens for the self-correction and for tracing this end-to-end — agreed with the reopen. **Blocker 1 (context-publish failure leaks a redeploy-blocking entry):** I agree with your read of `deployLocalTask`: once `executionContexts.put` / `cancellationFutures.put` run under `synchronized (this)` and `contextPublished` flips to `true`, a throw from `onContextPublished.run()` or either submit call bypasses `onFailureBeforeContextPublished`, so the entry stays behind. Since you note this is pre-existing baseline code shared with another branch, I'm fine tracking the fix in the separate issue you filed rather than blocking this PR on it. Two things on that: (1) your review body appears cut off mid-sentence after "either submit call" — could you paste the rest so the record here is complete? (2) Please confirm whether any of this PR's ownership changes (the `ownedContext` / `finishOwnedResources()` path) depend on that leaked entry being absent; if so we should at least note that dependency here so the follow-up doesn't regress it. **Blocker 2 (stale generation's own async/timer futures not cancelled):** this is the same as my earlier F4 (stale-cleanup branch recycles class loaders but never cancels the old generation's async functions or timer-flush tasks). It's specific to this diff and remains a blocker for this PR. Since you've confirmed the head is unchanged from `370f175e879` through `18e821c3d`, the rest of my earlier findings are also still open as far as I can tell: - F1/F3: `finishOwnedResources()` javadoc says it runs under the deployment monitor, but the context/cancellation-future installation in `deployLocalTask` doesn't actually close the redeploy-vs-taskDone race on `cancellationFutures`; either take the monitor there or fix the javadoc and the race separately. - F2: `BlockingWorker` should resolve its context from the tracker's `ownedContext`, not the shared `executionContexts` map, to avoid picking up a newer generation for a reused `TaskGroupLocation`. - F5: the plain `put` overwrites a still-live older generation's entry/future silently. - F6: heavy teardown (class-loader release, async/timer cancellation) under the service-wide monitor. - F7: `TaskDeployStaleContextRaceTest` doesn't exercise the `cancellationFutures` race and relies on reflection into internals. - F8: stale path never moves `ownedContext` into `finishedExecutionContexts`; redundant `get` before `remove(key, value)` in `finishExecutionContext()`. Concrete asks before I can re-approve: a new head that addresses Blocker 2/F4, F1/F3, F2 and F5 (F6–F8 can be follow-ups if you'd prefer, just say so), plus the completed text of the Blocker 1 write-up. <!-- streview-comment:875 --> -- 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]
