SEZ9 commented on PR #12311: URL: https://github.com/apache/seatunnel/pull/12311#issuecomment-6051307423
Thanks for spelling the matrix out. That is the behaviour I was hoping for on PR12311-F1 and PR12311-F2, but before I mark either resolved I need to see it in the diff on `f85d43600df`. Could you point me at the change in `resolveLostMemberState` that keys the resolution on the job status, and at `engineInitiatedCancelStillResolvesToFailed` and `notYetCancelledVerticesOfUserCancelledJobResolveToCanceled` in `CoordinatorServiceLostMemberResolutionTest`? For F2, please also confirm whether the job-status/vertex-state read now happens under the vertex lock, or explain why the window between the read and the resolution is benign. On the lower-severity items: - **F3** – is the "node offline" reason now logged or recorded anywhere when a vertex resolves to `CANCELED` via member loss? A single log line would be enough. - **F4** – has the restore Javadoc been updated to mention that a `FAILING` pipeline with `failedTaskNum == 0` stays `FAILED` only because of the `FAILING` pipeline-state check in `SubPlan#getPipelineEndState`? - **F5 / F6** – `makeTasksFailed` and `failedTaskOnMemberRemoved` now also resolve vertices to `CANCELED`; a rename, or at minimum a Javadoc note on the `Optional.empty()`-means-skip contract of `resolveLostMemberState`, would help, and the IT Javadocs that cite `failedTaskOnMemberRemoved` by name need the same touch. - **F7** – if resolution is now job-status based, the "regardless of which side of the ack" wording in `SplitClusterFaultToleranceIT` may now hold for `RUNNING` siblings too; please confirm whether that Javadoc was revisited, or point me at the current wording. On CI: understood that run 37337325279 produced no test results because the `Dead links` job failed on the `deepwiki.com` 429 and the test jobs were skipped behind it. I will hold off merging until there is a green run of `SplitClusterFaultToleranceIT` and `CoordinatorServiceLostMemberResolutionTest` on this head; please ping here once it has been re-run. <!-- streview-comment:1597 --> -- 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]
