SEZ9 commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5564184826
Thanks @DanielLeens for the CI triage on b6f8040dc and 0c2bbfe03 — noted that the failures were judged outside this PR's diff and that the retriggers (0c2bbfe0, 39312b92) were empty commits with no code change. Since there has been no code change since the review, the previous findings remain open: - **PR11757-F1 / PR11757-F4** — Future cancellation, `cancelAsyncFunction`, and classloader release run inside the service-wide monitor shared with `deployLocalTask`. Cancelling `CompletableFuture`s under the lock executes their dependent callbacks synchronously while every deploy on the node is blocked. Please only mutate the maps under the monitor and perform the actual cancellation/release after releasing it. - **PR11757-F2** — In the stale branch of `finishExecutionContext`, the old generation's async-function futures and timer-flush tasks are never cleaned up because those maps carry no generation identity. Please key them by generation (or execution context), or clean them up via the owning context. - **PR11757-F3** — With `cancellationFutures.put` now before `onContextPublished.run()`, a throw from the publish hook or a later submit leaves a live entry that the existing rollback path does not remove. Please add the `cancellationFutures` removal to the rollback path, or restore the original ordering. - **PR11757-F6** — `deployLocalTask` overwrites the `cancellationFutures` entry unconditionally, orphaning a possibly-live cancellation future of the previous generation. Please guard the overwrite, e.g. complete/cancel the previous future first or handle an existing entry explicitly. - **PR11757-F5** — `TaskExecutionServiceTest` injects entries into the shared service's internal maps and never removes them. Please add cleanup so state does not leak into subsequent tests on the shared server. - **PR11757-F7 / PR11757-F8** — The `get()` + `remove(key, value)` double check relies on all `executionContexts` writers holding the monitor, which is neither documented nor enforced; if they do, the `remove`-failed branch is unreachable, and the stale-generation warning is misleading when no active generation exists. Please either document/enforce the invariant and simplify the check, or make the log message accurate for that case. Happy to re-review once a commit addressing these lands. <!-- streview-comment:852 --> -- 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]
