SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5690933952
Thanks for the scope correction, @abdessalems — that's a fair point, and I'll go with it. Given that `finishOwnedResources()` and `testStaleTaskDoneCleansOnlyOwnedGenerationResources` are not in this PR's diff, I'm treating F1, F3 (the monitor/Javadoc claims) and F5 (the plain-put overwrite in `deployLocalTask`) as out of scope for #11727 and will follow them up against the change already on `dev` rather than block here. Same for the coverage half of F7 — the redeploy-vs-taskDone test on `cancellationFutures` living elsewhere is fine by me. What I still need from you on this PR's own diff: 1. **F2** — since `BlockingWorker.run()` is the core change here, please confirm (or point me at the lines) that it now resolves the context from the tracker's `ownedContext` rather than the shared `executionContexts` map, so a reused `TaskGroupLocation` can't pick up a newer generation's class loader. 2. **F7 (reflection part)** — `TaskDeployStaleContextRaceTest` still reaches into `TaskExecutionService` internals via reflection. If there's a way to drive the deploy-returns contract through a package-private hook or the existing test helper instead, I'd prefer that; if not, a short comment in the test explaining why reflection is required is enough. 3. **F4 / F6 / F8** — please confirm whether the stale-cleanup branch, the teardown-under-monitor path, and `finishExecutionContext()` are touched by this diff at all. If they aren't, I'll move those to the same follow-up as F1/F3/F5 and they won't hold this PR. Once I have those three answers I can do my final pass. <!-- streview-comment:1072 --> -- 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]
