SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-6229111737
Thanks for the follow-ups. Checked against head `0cd5d8544e4`; the only change since `d52d546763` is comments-only, so logic and tests are unchanged from the previously reviewed version. - **F1** – resolved. `cancelJob()` captures `jobStatus` once, guards with `isEndState()`, and branches on `NOT_STARTED_STATUSES.contains(jobStatus)`, mirroring `stopJob()`. The second `runningJobStateIMap.get(jobId)` read is gone. - **F3** – resolved. `JobStatusTest#testOrdinalOrderIsStableForInternalRpcCompatibility` now carries a Javadoc telling a future contributor to append new statuses rather than refresh the expected list, and `StateTransitionCleanupTest` covers the `PhysicalPlan` decision (`testCancelJobCommitsToTheJobStatusSnapshotItValidated`, `testCancelOnNotStartedJobGoesStraightToCanceled`, `testCancelOnRunningJobGoesThroughCanceling`). The first of those fails if a second IMap read is reintroduced. - **F4** – resolved. `NOT_STARTED_STATUSES` has Javadoc tying it to the state machine and pointing at the follow-up issue, and the duplicated call-site comments are removed. - **F2** – agreed to keep deferred to the follow-up issue. The check-then-act window outside the `PhysicalPlan` monitor is out of scope for this PR. Nothing else open from my side on the source. Remaining gates: a green CI run on `0cd5d8544e4` (the fork workflows need to be re-enabled first), and a committer approval, since my account is comment-only. <!-- streview-comment:1827 --> -- 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]
