SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5724749352
Thanks @CryoThrust for putting both answers into the branch at `2b0624991`. **F1** — The two tests you describe (`testCancelJobCommitsToTheJobStatusSnapshotItValidated` pinning the single-snapshot property with a mock IMap, and `testCancelJobOnClearedJobStatusFailsAtTheEndStateGuard` pinning the pre-existing NPE at the `isEndState()` guard) sound like exactly the right shape, and I agree that leaving the null path unchanged is the right scope call here. I haven't gone through the diff at `2b0624991` yet, so I'll confirm against the code before marking F1 resolved — but from your description I don't expect anything further on it. Still open before I approve: 1. **F2** — I don't see anything in the thread yet about the not-started check-then-act in `cancelJob()`/`stopJob()` running outside the `PhysicalPlan` monitor. A concurrent `startJob()`/`stateProcess()` could still move the job to `SCHEDULED`/`RUNNING` between the `NOT_STARTED_STATUSES` check and `updateJobState(CANCELED)`. Could you either bring that check-then-act under the same synchronization the transition path uses, or explain why that window is already guarded? 2. **F3** — Do the two classification tests you mention exercise the `NOT_STARTED_STATUSES` decision in `PhysicalPlan` itself (a not-started status going straight to `CANCELED`, a started one going through `CANCELING`)? And could you add a short note on the enum-pinning test in `JobStatusTest` saying what a future contributor must update when it trips? 3. **F4** — Please add a brief Javadoc on `NOT_STARTED_STATUSES` tying it to the state machine, and drop the inline comments at the two call sites that restate the set's contents, if they're still there. Happy to take another pass once those are in. <!-- streview-comment:1145 --> -- 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]
