DanielLeens commented on PR #12311: URL: https://github.com/apache/seatunnel/pull/12311#issuecomment-5846320065
Thanks for the thorough re-trace — I went back through `CoordinatorService`, `SubPlan`, and the addPhysicalVertexCallBack path against `ca60502` and Issues 1, 2, 3, and 4 all hold up; I'm not going to relitigate them. **Issue 1 (master-failover CANCELING treated as a user cancel):** Conceding this one fully. `resolveLostMemberState` only sees `ExecutionState`, and `restorePipelineState` reaches `PipelineStatus.CANCELING` without ever going through a user cancel or `neverNeedRestore`. Once restore is exhausted that path now ends CANCELED with the reason dropped, where it used to end FAILED with "node offline" — that's a real regression in what the job reports, not just a cosmetic gap. Your suggestion to gate on `jobMaster.getPhysicalPlan().getJobStatus() == JobStatus.CANCELING` is the right shape; `failedTaskOnMemberRemoved` already has `jobMaster` in scope so it's a cheap parameter to thread through. **Issue 2 (sibling vertices not yet reached by the cancel loop):** Also conceding. `SubPlan#stateProcess` cancels coordinator vertices before reader vertices, sequentially, each holding its own vertex lock through the un-timed RPC — so at the exact moment `memberRemoved` fires, a vertex the loop hasn't reached is still RUNNING, and `resolveLostMemberState` still fails it. This PR closes the "already-CANCELING" half of the window, not the whole thing. goutamadwant's 0/20 local result is encouraging but doesn't contradict you — it just means the loop is fast enough on a synthetic single-worker fixture that it usually wins the race, not that the race is gone. I agree with checking pipeline status instead of only vertex state: I'll change the resolver to take the owning `SubPlan`'s `PipelineStatus` (or thread the job-status flag from Issue 1 through the same call) so DEPLOYING/RUNNING vertices under an already-CANCELING pipeline also resolve CANCELED, while a genuinely FAILING pip eline still fails them. On the unsynchronized-read point: agreed it's a real TOCTOU, and while the plain read predates this PR, I'll fold the read-and-decide into a single synchronized path on `PhysicalVertex` while I'm touching this anyway, rather than leaving it as a separate follow-up. **Issue 3 (dropped node-offline reason on the CANCELED branch):** Agreed — I'll add a WARN in `makeTasksFailed` for the resolution it applies (task group, lost address, resolved state), symmetric with the WARN `noticeTaskExecutionServiceCancel` already emits for the pre-ack fallback, so a member-loss-driven cancel is distinguishable from a clean one in the logs. **Issue 4 (Javadoc's restore rationale):** Agreed, the guard that actually keeps a no-task-failure FAILING pipeline ending FAILED is `getPipelineEndState`'s explicit `PipelineStatus.FAILING` check, not `failedTaskNum`. I'll reword that sentence to name the real guard instead of implying the count is what saves it. **Issue 5 (naming/`null` sentinel on the private helper) and Issue 6 (`failedTaskOnMemberRemoved` name/Javadoc):** Both fair. Since I'm already reworking `resolveLostMemberState`'s signature for Issues 1 and 2, I'll take the opportunity to rename it and make the "leave the vertex alone" case explicit (`Optional` or a small enum) rather than an unlabeled `null`. For the public entry point I'll add the one-line Javadoc noting the CANCELING→CANCELED branch rather than renaming it — it's cited by name from five IT Javadocs and a rename would be pure churn for no behavior change. **Issue 7 (IT Javadoc overstating the guarantee):** Conceding the wording is too strong today — it currently only holds for the vertex that's already CANCELING, exactly as you traced through `awaitAnyVertexCanceling` and the sequential cancel order. I'll scope it down for now, and once the Issue 1/2 pipeline-status gate lands, the stronger wording will actually be true for the whole pipeline rather than just the first-cancelled vertex, so I'd rather fix the guarantee itself than just soften the sentence permanently. I'll push a revision covering all seven points together rather than trickling out partial commits, and will flag this thread again once it's up so the CANCELING/CANCELED mapping can get a fresh look against the new diff. Thanks for the rigor here — Issues 1 and 2 in particular were real gaps I'd missed. -- 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]
