SEZ9 commented on PR #12311: URL: https://github.com/apache/seatunnel/pull/12311#issuecomment-6621577126
Thanks for the re-review pass on `65ef1bde182c8ef9e0f8f0da03e2c29fc7517a42`. Resolving the lost member's vertices based on the job status rather than the per-vertex state, with the read under the vertex monitor, addresses the core of what was flagged on `ca60502b9e`. Going through the earlier points against this head: **F1 (engine-initiated CANCELING → CANCELED):** Gating on the job status should keep a master-failover reschedule out of the CANCELED branch, but I'd like that confirmed rather than assumed. Could you walk through what `JobMaster#getJobStatus` returns while `SubPlan#restorePipelineState` is cancelling tasks after a master failover, and point to the test (new or existing) that exercises that path and asserts the job is rescheduled / ends FAILED rather than CANCELED? **F2 (RUNNING siblings, read outside the lock):** Using the job status means siblings the sequential cancel loop hasn't reached yet resolve to CANCELED too, and the read now happens under `synchronized (physicalVertex)` — good. One remaining ask: is there a test where the lost member hosts both a CANCELING vertex and a still-RUNNING vertex, asserting the final job state is CANCELED? If `testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck` already covers that shape, a short note in its Javadoc is enough. **F3 (dropped "node offline" reason):** The WARN when member loss completes a cancel covers this. Please make sure it includes the job id, pipeline id, task group / vertex id and the removed member address so operators can correlate it. **F4 (Javadoc restore argument):** Please update the restore Javadoc to state that a pipeline that went FAILING with `failedTaskNum == 0` stays FAILED because `SubPlan#getPipelineEndState` checks the FAILING pipeline state, not the failed-task count. **F5 / F6 (naming):** `resolveTasksOnLostMember` plus `Optional<ExecutionState>` from `resolveLostMemberState` reads much better than the `null`-means-skip contract. `failedTaskOnMemberRemoved` is still the public entry point and still says "failed" while it now also resolves vertices to CANCELED. If you'd rather not rename it in this PR, please at least update its Javadoc and the IT Javadocs that reference it by name so they describe both branches. **F7 (IT Javadoc overstating the guarantee):** With the job-status gate, the "either way the vertex reaches CANCELED" statement is closer to true, but please reword it to say explicitly that the guarantee rests on the job being in CANCELING at the time of member loss, and that this is why it also covers RUNNING siblings. Once the F1 trace/test and the F2 sibling coverage are in place, the rest are doc/naming touch-ups and I'm happy to do a final pass. <!-- streview-comment:2265 --> -- 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]
