DanielLeens commented on PR #11602: URL: https://github.com/apache/seatunnel/pull/11602#issuecomment-5678685460
Thanks @SEZ9 for the second pass, and thanks @tomatotomata for the departed-worker fix — I agree with the technical read: treating a member that has already left the cluster as unavailable, instead of spending the retry budget on a node that can never respond, is the correct minimal fix, and it matches what I found tracing the same diff in my own last review on this exact head (`a283df0d52`). I do want to flag something important though, since it changes the picture on the one item I'm still blocking on. In that same last review I called out that `SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck` was still red on this head — not the 60s timeout from before, but a genuine `expected: <CANCELED> but was: <FAILED>` assertion mismatch — and I said I couldn't yet tell whether that lives in this PR's own changes or is a pre-existing `dev`-level defect this PR is incidentally exposing now that the metrics-retry mask is gone. I've since root-caused it in a separate pass and can now answer that question directly: it reproduces on plain `dev` itself, unrelated to anything in this PR. The short version — when a worker dies *after* acking a `CancelTaskOperation` but *before* its completion callback fires, `noticeTaskExecutionServiceCancel` has already returned normally (vertex left `CANCELING`), and Hazelcast's `memberRemoved` → `failedTaskOnMemberRemoved` → `makeTasksFailed` path explicitly matches `CANCELING` vertices and marks them `FAILED` before the intended `CANCELED` fallback ever gets a chance to run. Which side of the ack the worker dies on is pure scheduling timing, so it's a genuine flake rather than a deterministic bug — I found it failing on `dev`'s own push CI on every completed run since 2026-09-13 (roughly a 70% failure rate), and I've opened #12311 with a fix and the full trace if anyone wants the details. So: Issue 1 from my last review is real, but it isn't this PR's bug, and it isn't something this PR needs to fix — it's `dev`-level instability from an unrelated race that predates and is independent of this branch. I don't want to leave that open against this PR any longer than necessary now that I have the evidence to say so with confidence. Since there's no new commit on this PR since my last full review, I'm not publishing a fresh formal review in this reply per our usual process — but this does mean I expect my "Not recommended for merge" conclusion to change once I do the next full pass (either after #12311 lands and this branch is rebased past it, or after a fresh CI run that isn't dragged down by that same flake). If it'd help move things along, syncing this branch with the latest `dev` once #12311 merges would be the cleanest way to get a clean `engine-v2-it` signal on the next run. On the local-build question — that one's really for @tomatotomata to answer, since I haven't reproduced the `seatunnel-config-shade` compile-error report myself on either a clean `dev` checkout or this branch, so I don't have direct evidence either way. Would still be good to see the first error either way, mainly to rule out an environment-specific JDK/toolchain issue versus something in the module graph. -- 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]
