SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5674083272
Thanks for the update on `bf6870507`. Here is where I am on the earlier points: **F1 (second unsynchronized IMap read in `cancelJob()`)** — Could you point me to the hunk that replaces the `runningJobStateIMap.get(jobId)` lookup with the already-captured `jobStatus` local, and confirm the null-state path in `cancelJob()` behaves as it did before this PR? I'd like to verify this against the code before marking it resolved. **F2 (check-then-act outside the PhysicalPlan monitor)** — I don't have a status on this one yet. Are the not-started check and the subsequent `updateJobState(CANCELED)` in `cancelJob()`/`stopJob()` now performed under the same lock as `startJob()`/`stateProcess()`, or is there a reason the race (job moving to SCHEDULED/RUNNING between check and update) is not reachable in practice? Either a code change or a short justification here is fine. **F3 (test coverage)** — The ordinal-order guard in `JobStatusTest` is good. Two remaining asks: (a) a test that exercises the `NOT_STARTED_STATUSES` decision in `PhysicalPlan` itself (e.g. cancel on a PENDING job goes straight to CANCELED, cancel on a RUNNING job does not), and (b) an assertion message or comment on the guard test telling a future contributor what to do when it fails (append new constants at the end, never reorder, since the ordinal is the RPC wire value and the timestamp array index). **F4 (Javadoc / duplicated comments)** — Please add a Javadoc on `NOT_STARTED_STATUSES` describing it as the set of states before `startJob()` has scheduled any pipeline, and drop or shorten the inline comments at the two call sites so the set contents are documented in one place. Once F1 is confirmed in the diff, F2 is answered, and F3/F4 are in, I'm happy to approve. <!-- streview-comment:1060 --> -- 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]
