SEZ9 commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5611965989
@abdessalems thanks for the point-by-point status on e66911c0c — that's exactly what I needed, and apologies for the delayed answer. Where I land on each: - **F2 / F4 / F6 / Issue 2** — the described changes (ownedContext for BlockingWorker, the OwnedFuture tagging so the stale branch only cancels its own generation's async-function and timer-flush entries, teardown moved outside the `TaskExecutionService.this` monitor, and the self-guarding `cancellationFutures.remove(location, ownedCancellationFuture)`) address what I raised. I'll confirm against the diff on re-review, but nothing further to ask on these. - **F8** — fine with the redundant `get` removed and with the stale branch deliberately not recording into `finishedExecutionContexts`; a one-line comment at that branch stating it's intentional (because of key reuse) would save the next reader the same question. - **F7** — `testStaleTaskDoneCancelsItsOwnAsyncAndTimerFutures` and `testDeployTaskIdempotentWhenAlreadyRunning` as deterministic coverage is the better approach. One question: do the new tests still reach into `TaskExecutionService` internals via reflection, or did you get a non-reflective seam for them? - **F1 / F5** — accepting the deferral to #12164 since it's shared baseline code. Please add a short note in the PR description that the redeploy-vs-taskDone publish race is tracked there, so nobody reads this PR as closing it. - **F3** — this one I'd still like handled here rather than in #12164: the `finishOwnedResources()` Javadoc is text introduced by this PR, and as long as `deployLocalTask()` doesn't take that monitor, the "under the same monitor used by deployment" wording is inaccurate. Please reword it to describe what the monitor actually covers now (the map/future bookkeeping), and reference the follow-up for the deployment side. So the remaining asks are: the F3 Javadoc fix, the F8 comment, the F7 reflection answer, and the description note for F1/F5. Once the F3 wording is pushed I'll do the re-review. <!-- streview-comment:942 --> -- 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]
