SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-6160403682
Thanks @CryoThrust for the sync to `b3898b62d` and the verification notes on `d52d54676`. **F1** – `cancelJob()` branching on the captured `jobStatus` local, matching `stopJob()`, is exactly what I asked for. Good. **F4** – Javadoc on the constant plus removal of the duplicated call-site comments covers this. **F3** – Partly. `testCancelJobCommitsToTheJobStatusSnapshotItValidated` and the not-started/running decision tests address the missing coverage of the `NOT_STARTED_STATUSES` decision. The other half of F3 was the enum-order guard in `JobStatusTest` carrying no explanation of what a future contributor should do when it fails. Your note mentions the cleared-status regression being documented, but not the guard itself. Could you confirm whether that guard now has such a comment, or add one? **F2** – Agreed on keeping the monitor/lock-order change in `cancelJob()`/`stopJob()` out of this PR and tracking it in the follow-up issue you referenced; the static outcome tests indeed do not prove the interleaving safe. One small ask: add a short pointer to that issue in the `NOT_STARTED_STATUSES` Javadoc (or next to the two call sites) so the deferred race is discoverable from the code, not only from this thread. Remaining before merge: 1. The F3 guard comment (or confirmation it is already there). 2. The follow-up issue pointer above. 3. A CI run on the current head. I understand the fork's workflows are currently paused; once they are re-enabled, please trigger a run and drop the link here. The local JDK 11 results (`StateTransitionCleanupTest` 13 passed, `JobStatusTest` 2 passed, Spotless and `-DskipTests verify` passing) are reassuring, but I want a green run on record before merging. <!-- streview-comment:1745 --> -- 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]
