SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5862310940
Thanks for the follow-up, @abdessalems. On the review side: the automated summary above says the three items I flagged earlier — the deploy/teardown monitor (finishOwnedResources() vs. deployLocalTask()), the `BlockingWorker` context source, and the stale-cleanup path — were addressed at `06c2ef03e`, and that the current head `a80834ebf` is byte-identical to `06c2ef03e` for the files this PR touches. I'm happy to take that as the working state, but I'd like to close the loop on the rest of that review before we talk about merging. Could you confirm, ideally with a short pointer to the relevant change in `TaskExecutionService.java` / `TaskDeployStaleContextRaceTest.java`, how each of these landed (or why you consider them not applicable): - the plain `put` in `deployLocalTask` overwriting a still-live older generation's context/cancellation future; - heavy teardown (class-loader release, async-function and timer-flush cancellation) running under the service-wide `TaskExecutionService.this` monitor; - the race test covering the redeploy-vs-taskDone race on `cancellationFutures` rather than only the deploy-returns contract, and whether the reflection into `TaskExecutionService` internals can be avoided; - the stale path not moving `ownedContext` into `finishedExecutionContexts`, and the redundant `get` before `remove(key, value)` in `finishExecutionContext()`. A one-line-per-item response is fine if they're already in the diff; I just don't want to rely on a summary that only lists three of them. On CI: I'll go with what @zhangshenghang said — we merge once CI passes. I understand your point that the engine-v2-it failure looks like the race tracked in #12311 rather than something this PR introduced, but I'd rather not merge past a red leg on a change that is itself about a deploy/teardown race. If #12311 stays open for a while, a re-run once it (or a fix for that flake) lands is the cleanest path; if you have evidence the failure is unrelated beyond the dev-merge observation, feel free to post it here and we can reassess. <!-- streview-comment:1365 --> -- 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]
