SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5707337983
Thanks @abdessalems for checking each point against the diff. 1. **F2** — The change you describe (the worker reading the class loader from the tracker's pinned context, with the shared-map lookup and null-context branch removed) is exactly what F2 asked for. I'll confirm it against the diff before marking it resolved. 2. **F7 (reflection)** — Understood. If the only non-reflective paths to the map tear the task group down rather than race the deploy, a package-private accessor would be production surface for test-only use, which I don't want. Keeping the reflection with the Javadoc on `executionContextsOf()` in 2dc214681 is fine by me. The coverage half of F7 is still open: the test exercises the deploy-returns contract but not the redeploy-vs-`taskDone` race on `cancellationFutures`. That depends on F1/F5 below. 3. **F4 / F6 / F8** — To make sure I read you correctly: "not touched by this diff" means the incremental push didn't change that code, not that it's outside this PR? Those findings were raised against code introduced or changed by this PR (the teardown under the service-wide monitor, the stale-cleanup branch, `finishExecutionContext`), so unless that code has been removed from the PR they remain open. If you think any of them is pre-existing behaviour, please point me at what shows that and I'll re-check. Still open before I can approve: - **F1 / F3**: does `deployLocalTask()` now install the context and cancellation future under the same monitor that `finishOwnedResources()` uses, or has the Javadoc been adjusted to match the actual locking? Neither shows up in your grep of the incremental diff, so I assume both are unchanged. - **F5**: is the plain `put` in `deployLocalTask` still there? If so, I'd like either an explicit replace with a log line, or a short note on why overwrite is safe now that the worker no longer reads from the map. - **F7 (coverage)**: if F1/F5 need a code change, a test exercising redeploy-while-`taskDone` on the same location would close this out; if they're argued safe, a comment in the test pointing to that argument is enough. Happy to take another look once those are addressed. <!-- streview-comment:1097 --> -- 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]
