SEZ9 commented on PR #12311: URL: https://github.com/apache/seatunnel/pull/12311#issuecomment-5862478155
Thanks for the fix, and thanks @davidzollo for the detailed verification. Before merging, a few points from my earlier review are still open: 1. **Engine-initiated cancel.** `CANCELING` is not only user-initiated: `SubPlan#restorePipelineState` also cancels tasks during a master-failover reschedule. With this change, a job the user never cancelled can end as `CANCELED` with no error reason instead of `FAILED`. Could you either distinguish the two cancel origins in `resolveLostMemberState`, or explain why that path cannot reach `makeTasksFailed` with a `CANCELING` vertex? 2. **Sibling vertices still `RUNNING`.** The resolution consults only the vertex state, and reads it outside the vertex lock. Vertices on the lost worker that the sequential cancel loop has not reached yet are still `RUNNING`, so they resolve to `FAILED` and flip the user-cancelled job to `FAILED` anyway. Please consider consulting the pipeline/job state (and taking the lock) so the fix covers the whole pipeline rather than only vertices that already entered `CANCELING`. 3. **IT Javadoc guarantee.** Related to the above: "Either way the vertex reaches CANCELED ... regardless of which side of the ack" only holds for vertices already in `CANCELING`. Please tighten the wording, or make it true via point 2. 4. **Dropped "node offline" reason.** The reason attached to a `CANCELED` resolution is never logged, and `SubPlan` only records throwable messages for `FAILED` tasks. A log line at the resolution site would let operators tell a member-loss cancel from a clean cancel. 5. **Javadoc restore argument.** Please note that a pipeline that turned `FAILING` without any task failure has `failedTaskNum == 0` and stays `FAILED` only because `SubPlan#getPipelineEndState` checks the `FAILING` pipeline state. 6. **Naming.** `makeTasksFailed` and the public `failedTaskOnMemberRemoved` now also resolve vertices to `CANCELED`, and `resolveLostMemberState` uses `null` to mean "skip". A rename, a short Javadoc for the null contract, and updating the IT Javadocs that cite the old name would make the branching harder to misread. Points 1 and 2 are the ones I'd like addressed (or argued away) before merge; the rest are small follow-ups that can land in the same PR. <!-- streview-comment:1372 --> -- 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]
