DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-6383278055
Thanks @SEZ9 for the careful follow-up. I fully agree on the CI part: the `engine-v2-it` failures (`BackpressureSlowSinkIT` and `CheckpointCoordinatorFailoverIT`) sit close to the task-lifecycle code, so a same-scenario baseline run on `dev` is still needed before anyone calls them unrelated. That ask stands for @abdessalems. On the source items, I re-read the current head `a80834ebfb185ee0d54243e7a3ae99a5bf481817` again, and several of the points still listed as open describe code that is not in this version. To keep the thread from drifting, here is what this head actually does: - **F1 / F3 / F5 (deploy vs. taskDone):** there is no `finishOwnedResources()` method and no Javadoc in this head that claims a shared `TaskExecutionService.this` monitor; `git grep` finds neither `finishOwnedResources` nor `synchronized (TaskExecutionService.this` in the file. The deploy side is documented as lock-free on purpose ([lines 705-722](https://github.com/apache/seatunnel/blob/a80834ebfb185ee0d54243e7a3ae99a5bf481817/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/TaskExecutionService.java#L705-L722)). It registers the execution-scoped resources first and publishes `location -> context` last. `cancellationFutures` is keyed by `TaskGroupContext` ([line 217](https://github.com/apache/seatunnel/blob/a80834ebfb185ee0d54243e7a3ae99a5bf481817/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/TaskExecutionService.java#L217)), not by location, so a new deployment's `put` cannot overwrite an older generation's cancellation future. The only location-keyed `put` is the `executionContexts` publish, where a newer generation is meant to supersede the older one. The older tracker then cannot remove it, because `finishExecution()` only clears the mapping when `context.equals(activeContext)` inside `executionContexts.compute(...)` ([lines 1689-1701](https://github.com/apache/seatunnel/blob/a80834ebfb185ee0d54243e7a3ae99a5bf481817/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/TaskExecutionService.java#L1689-L1701)). - **F2 (BlockingWorker context):** `BlockingWorker.run()` reads `taskGroupExecutionTracker.context.getClassLoaders()` ([lines 1285-1294](https://github.com/apache/seatunnel/blob/a80834ebfb185ee0d54243e7a3ae99a5bf481817/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/TaskExecutionService.java#L1285-L1294)), which is the context the tracker pinned at deploy time. It does not look anything up in the shared `executionContexts` map. - **F4 (stale-branch cleanup):** the stale branch does not return early. After the conditional `compute`, `finishExecution()` always removes this context's cancellation future, recycles its class loaders, and cancels its async-function and timer-flush futures ([lines 1703-1728](https://github.com/apache/seatunnel/blob/a80834ebfb185ee0d54243e7a3ae99a5bf481817/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/TaskExecutionService.java#L1703-L1728)). The superseded case only logs a warning. - **F8 (`finishedExecutionContexts`):** only the active generation is moved into `finishedExecutionContexts`. That map is keyed by `TaskGroupLocation`, so putting a superseded generation there would record a stale context under a location the newer generation owns. If you think the superseded generation should be recorded somewhere, I am open to discussing it, but this is a design choice and not a missing cleanup step. There is also no `finishExecutionContext()` method with a redundant `get` in this head. - **F6 (teardown under the service monitor):** none of the teardown runs inside a service-wide `synchronized` block. The only atomic section is the per-location `compute`, and class-loader recycling plus future cancellation run after it. - **F7 (test coverage):** this one I agree with as a reasonable non-blocking improvement. `TaskDeployStaleContextRaceTest` states that it covers only the deploy-return contract ([lines 65-85](https://github.com/apache/seatunnel/blob/a80834ebfb185ee0d54243e7a3ae99a5bf481817/seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/TaskDeployStaleContextRaceTest.java#L65-L85)). A direct redeploy-vs-`taskDone()` test using package-private hooks instead of reflection would strengthen it. If any of F1-F6/F8 refers to a specific line on this head that I am reading differently, please point to the exact line and I will gladly re-check it. Otherwise, I think the remaining open items for @abdessalems are the baseline comparison for the two engine IT failures and, optionally, the F7 test. This is not a new approval or a claim that the PR is ready to merge while `Build` is still red. -- 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]
