SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5564126990
Thanks @DanielLeens for the CI triage on 23bbcb526, b555e0a9a and 18e821c3d — noted that the failures listed there are outside this PR's diff and that the retriggers (b555e0a9, 18e821c3) were empty commits with no code change. With no code change in those retriggers, the earlier review findings on `TaskExecutionService` and `TaskDeployStaleContextRaceTest` remain open. To keep the review moving, here is what I still need: 1. **F1 / F3 / F5 (redeploy vs. taskDone race, monitor claim, plain `put`)** — please have `deployLocalTask()` install the context and cancellation future under the same monitor that `finishOwnedResources()` uses, and replace the plain `put` with a guarded install that refuses to silently overwrite a still-live older generation. Update the `finishOwnedResources()` Javadoc so it matches the actual locking once that is done. 2. **F2 (BlockingWorker context resolution)** — resolve the context from the tracker's `ownedContext` rather than the shared `executionContexts` map, so a reused `TaskGroupLocation` cannot pick up a newer generation's context/class loader. 3. **F4 / F8 (stale-cleanup branch)** — in the stale path, cancel the old generation's async functions and timer-flush tasks in addition to recycling class loaders, move `ownedContext` into `finishedExecutionContexts`, and drop the redundant `get` before `remove(key, value)` in `finishExecutionContext()`. 4. **F6 (heavy teardown under the service-wide monitor)** — please keep only the map/future bookkeeping under the `TaskExecutionService.this` monitor and run class-loader release and cancellations outside of it. 5. **F7 (test)** — extend `TaskDeployStaleContextRaceTest` to exercise the redeploy-vs-taskDone race on `cancellationFutures`, and if possible reduce the reflection into `TaskExecutionService` internals. Once those changes are pushed, a short note here summarizing how each finding was addressed would be great, and I'll re-review promptly. <!-- streview-comment:851 --> -- 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]
