SEZ9 commented on PR #12377: URL: https://github.com/apache/seatunnel/pull/12377#issuecomment-6246139637
Re-checked against `24b817941c53`; I think the two views reconcile, so let me narrow the asks. **PR12377-F1 (CANCELING guard / FAILING overwrite):** "unreachable" was too strong on my side. For a vertex that has already had `cancel()` invoked, the synchronized path through `stateProcess()` -> `noticeTaskExecutionServiceCancel()` holds the vertex monitor until it self-resolves to `CANCELED`, and a late `updateStateByExecutionService(FAILED)` is then dropped by the `isEndState()` guard, so the fix does close that ordering. The remaining gap is the sibling case: a still-`RUNNING` vertex on the same crashed worker that the cancel loop hasn't reached yet can be completed as `FAILED` via `failedTaskOnMemberRemoved()` -> `makeTasksFailed()`, which drives `addPhysicalVertexCallBack` to bump `failedTaskNum` and call `updatePipelineState(FAILING)` before `determinePipelineEndState()` ever sees `CANCELING`. That ordering still ends the pipeline as `FAILED`. Ask: either (a) have the callback honor an in-flight cancel (e.g. check whether the pipeline is already `CANCELING` before transi tioning to `FAILING`), or (b) mark all vertices as cancel-requested before any of them can be failed by member removal. Please describe the chosen ordering in the PR description. **PR12377-F2 (test):** `SplitClusterFaultToleranceIT#testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck` covers the mid-cancel vertex, but the hand-crafted `CANCELING + failedTaskNum=1` unit test still bypasses the callback that does the overwrite. Ask: add a case (unit or IT) for the sibling-vertex ordering above — a pipeline with at least two vertices on the crashed member, one still `RUNNING` when the member is removed — and assert the pipeline ends `CANCELED`. It should fail on the current head and pass with the F1 fix. **PR12377-F3 (visibility/Javadoc):** unchanged and minor — please add the `VisibleForTesting` annotation and a short Javadoc on `determinePipelineEndState()` describing the precedence between cancel and failure, since that is the contract the new tests rely on. Happy to re-review once the sibling-vertex ordering is handled. <!-- streview-comment:1850 --> -- 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]
