DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5881982649
Thanks for pushing for a real confirmation here, @SEZ9 - a three-item summary doesn't cover what you originally raised, fair call. I checked all four items directly against the current head (`a80834ebf`, byte-identical to `06c2ef03e` for the three files this PR touches): 1. **The plain `put` in `deployLocalTask` overwriting a still-live older generation's context/cancellation future** - this is explicitly out of scope for this PR, not resolved by it. `TaskDeployStaleContextRaceTest.java:80-85` says so directly: "the other half of the same family - a redeploy for this location racing an in-flight `taskDone()`, which can evict or tear down the newer deployment's cancellation future, async functions and timer flushes - lives in `deployLocalTask()` and the tracker teardown, neither of which this test nor the fix it covers touches... That path is the design of #12238 and is tracked against it." So: not addressed here, tracked separately under #12238. What this PR closes is the other half of the family - the deploy-side hang from #11679. 2. **Heavy teardown running under the service-wide `TaskExecutionService.this` monitor** - not the case at the current head. `finishExecution()` (`TaskExecutionService.java:1685-1729`) does the active-to-finished transition via `executionContexts.compute(...)` (line 1692), which only locks that map's internal bucket for that one key, not the service-wide monitor. `recycleClassLoader`, `cancelAsyncFunctionFutures` and `cancelTimerFlushFutures` (lines 1714-1728) all run afterward, outside any `synchronized` block. So class-loader release and async/timer-flush cancellation are not held under a service-wide lock. 3. **Race test covering the redeploy-vs-taskDone race on `cancellationFutures`, and the reflection** - same answer as (1): the test class javadoc says this race is explicitly out of scope for this PR and deferred to #12238; it isn't silently unaddressed, it's a documented boundary. On the reflection: `TaskDeployStaleContextRaceTest.java:280-289` gives the reason - `executionContexts` is private, and the only public ways to remove an entry (`deployTask`, `cancelTaskGroup`) tear down the whole task group rather than just the map entry, which would stop the deployment being raced instead of racing it. Adding a package-private accessor purely for this test would widen the production surface for test-only visibility, so the reflection is a deliberate, documented tradeoff rather than an oversight. 4. **The stale path not moving `ownedContext` into `finishedExecutionContexts`, and a redundant `get` before `remove(key, value)`** - first half is correct as read, and it is intentional: in `finishExecution()`'s `compute()` (lines 1692-1701), the stale branch (`!context.equals(activeContext)`) returns `activeContext` unchanged and never touches `finishedExecutionContexts` - only the branch that matches the currently active context moves itself into `finishedExecutionContexts` before returning `null`. A stale/superseded generation's context is deliberately dropped rather than archived, since it was never the result callers should observe for that location. Second half: I could not find a `get`-then-`remove(key, value)` pattern anywhere in `finishExecution()` or elsewhere in the file at this head - the only removal from `finishedExecutionContexts` is the unconditional single-key `remove` in `notifyCleanTaskGroupContext()` (line 889), and the active-context transition itself is one atomic `compute()` call, not a separate get + conditional-remove. If that pattern was visible on an earlier revision, it looks like it has already been folded into that `compute()`. On CI: agreed with where you and @abdessalems landed. I just re-checked - Build is still red on this head, `mergeable_state` is `blocked` on review/required-checks only (no conflicts reported), and #12311 is still open and unmerged, so the engine-v2-it diagnosis both of you gave still holds as of right now, not just as of the earlier comment. -- 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]
