SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5787767017
@CryoThrust thanks for the follow-up in `449b51782`. The clarification you describe on `testCancelJobOnClearedJobStatusFailsAtTheEndStateGuard` — pinning the pre-existing NPE on a cleared/null status as a regression tripwire rather than a desired design contract, so any future change to cleared-state handling is deliberate — is the kind of "what should a future contributor do when this fails" note I was after, and it fits the fact that the NPE predates the single-snapshot change in `cancelJob()`. To wrap up the remaining items from my earlier review, could you confirm the current state of these on the head commit (`449b517823f5dd4d231634eea32a52d8e5f191dc`, relative to `2b0624991`)? 1. **PR12156-F3** (enum-table guard test in `JobStatusTest.java`): does that test now carry a similar explanation of what to do when it fails, and is there a test that exercises the `NOT_STARTED_STATUSES` decision in `PhysicalPlan` itself rather than only the enum table? 2. **PR12156-F2** (check-then-act outside the `PhysicalPlan` monitor): is the not-started check plus `updateJobState(CANCELED)` in `cancelJob()`/`stopJob()` now performed under the same lock that `startJob()`/`stateProcess()` use, or is the position that the single-snapshot read is sufficient? A short note either way is fine; if the latter, please spell out why a concurrent transition to SCHEDULED/RUNNING between the check and the update cannot happen. 3. **PR12156-F4** (`NOT_STARTED_STATUSES` Javadoc / duplicated inline comments): does `NOT_STARTED_STATUSES` now have a Javadoc tying it to the state machine, and have the two call-site comments that repeated the set contents been trimmed to reference the constant instead? Once those are confirmed, I have nothing further on this PR. <!-- streview-comment:1241 --> -- 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]
