SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5673570163
@abdessalems thanks for the heads-up — no surprise on my side, and the F7 reflection point is settled as far as I'm concerned. Keeping the single reflective access into `executionContexts` (line 276) so the remover thread can pull an entry mid-deploy is exactly the trade-off I asked for: a narrow, test-setup-only hook rather than widening the production API for test visibility. The fact that none of the ownership-internals reflection remains and that the `TaskExecutionServiceTest` additions are reflection-free is what I wanted to hear. Acknowledged that the test Javadoc update landed in 01a705240 and the PR description was refreshed the same day; I'll pick those up in my pass. Two small asks to close out the review scope: 1. **F7, coverage half** — the original finding also noted the race test covered the deploy-returns contract but not the redeploy-vs-taskDone race on `cancellationFutures`. Could you point me at which of the new `TaskExecutionServiceTest` additions (or which case in `TaskDeployStaleContextRaceTest`) exercises that path, so I can verify it directly rather than infer it? 2. **F1/F3 wording** — when I re-read `finishOwnedResources()` I'll be checking that the Javadoc's monitor claim now matches what `deployLocalTask()` actually does around context installation. If you've already adjusted either the code or the comment for that, a one-line pointer saves me a hunt. I'll go through the remaining files on the current head and follow up here. <!-- streview-comment:1046 --> -- 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]
