SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5923571862
Thanks for the update, @abdessalems. Before we talk about merging past the integration-test failure you describe, I need to close the loop on the review findings from the last round — I don't see them addressed in this thread yet, and nothing in your comment speaks to them. Concretely, these are still open from my side: 1. **Redeploy-vs-taskDone race (PR11727-F1 / F3 / F5)** — `finishOwnedResources()` documents that it runs under the same monitor as deployment, but `deployLocalTask()` installs the context and cancellation future into the shared maps via a plain `put` without taking that monitor. That means the race on `cancellationFutures`, async functions and timer flushes is not actually closed, and the Javadoc is currently inaccurate. Please either take the `TaskExecutionService.this` monitor around the installation (or otherwise make the install/finish pair atomic) and make the `put` refuse to silently overwrite a still-live older generation, or update the Javadoc to describe what's really guaranteed. 2. **`BlockingWorker` context lookup (PR11727-F2)** — it still resolves its context from the shared `executionContexts` map rather than the tracker's `ownedContext`, so a worker can pick up a newer generation's context/class loader for the same reused `TaskGroupLocation`. Please route it through `ownedContext`. 3. **Stale-cleanup branch (PR11727-F4 / F8)** — after recycling the class loaders it returns without cancelling the old generation's async functions or timer-flush tasks, and it never moves `ownedContext` into `finishedExecutionContexts`. The redundant `get` before `remove(key, value)` in `finishExecutionContext()` can go at the same time. 4. **Teardown under the service-wide monitor (PR11727-F6)** — class-loader release and async-function/timer cancellation now run while holding `TaskExecutionService.this`. If the monitor is kept for the map mutations, please move the heavy teardown outside it. 5. **Test coverage (PR11727-F7)** — `TaskDeployStaleContextRaceTest` covers the deploy-returns contract but not the `cancellationFutures` race above, and reaches into internals via reflection. Once (1) is fixed, a test that exercises the redeploy-vs-taskDone path directly would be ideal. On the CI question itself: I can't verify from this thread which legs are red or why, and a flaky integration test on dev doesn't change the fact that the items above are correctness issues in this change. Once they're addressed (or you tell me why a given one doesn't apply), I'm happy to look at the CI situation with you and decide whether a dev-level flake should block. Could you push a revision covering the points above, or reply inline on any you disagree with? <!-- streview-comment:1439 --> -- 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]
