SEZ9 commented on PR #12311: URL: https://github.com/apache/seatunnel/pull/12311#issuecomment-6008991117
Thanks for the detailed point-by-point write-up on `f85d43600df`. The approach you describe lines up with what I was after on each of the earlier points: gating the `CANCELED` resolution on `JobStatus.CANCELING` rather than the vertex state alone, resolving `DEPLOYING`/`RUNNING` siblings while the job is `CANCELING`, doing the read, decision and transition inside `synchronized (physicalVertex)`, the WARN on a member-loss-completed cancel, naming `SubPlan#getPipelineEndState` as the real guard in the restore Javadoc, the `resolveTasksOnLostMember` rename with `Optional<ExecutionState>`, and the reworded IT guarantee. I haven't inspected the revision itself yet, so I'll confirm each of these against the code rather than close them here. One ask in the meantime: your comment appears cut off at "`CANCELING`/`DEPLOYING`/`RUNNING` under a `CANCELING` jo…". Could you post the rest of the matrix that `CoordinatorServiceLostMemberResolutionTest` pins? In particular I'd like to see that it covers the negative cases — a `CANCELING` vertex under a non-`CANCELING` job status (e.g. `RUNNING`, `FAILING`) resolving to `FAILED` — and which states are expected to be skipped. Once I have the full matrix I'll do a pass over the revision. <!-- streview-comment:1554 --> -- 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]
