DanielLeens commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5674339434
@SEZ9 this crossed with my own consolidated re-review and Approve on `bf6870507` (https://github.com/apache/seatunnel/pull/12156#pullrequestreview-5204280564), which answers all four of these directly — pulling the key parts out here so you don't have to dig for them: **F1 — hunk and null-state confirmation.** Base (`dev`, before this PR, `303a757489^`): ```java public void cancelJob() { JobStatus jobStatus = getJobStatus(); if (jobStatus.isEndState()) { ... return; } if (((JobStatus) runningJobStateIMap.get(jobId)).ordinal() <= JobStatus.PENDING.ordinal()) { ``` Current head (`bf6870507`, `PhysicalPlan.java:211-220`): ```java public void cancelJob() { JobStatus jobStatus = getJobStatus(); if (jobStatus.isEndState()) { ... return; } if (NOT_STARTED_STATUSES.contains(jobStatus)) { ``` So the second, independent `runningJobStateIMap.get(jobId)` read is gone; the ordinal/set check now consumes the exact same `jobStatus` local that `isEndState()` already validated. On the null-state path specifically: yes, unchanged from before this PR and now symmetric with `stopJob()` (which has always worked this way, in the base too) — if `getJobStatus()` returns null (IMap entry concurrently cleared), `jobStatus.isEndState()` NPEs at the top of the method, before either branch is reached. That NPE-on-null behavior isn't new; what F1 actually fixed was the second read potentially observing a *different, non-null* status than the one `isEndState()` validated, not the null case itself. **F2** — not folded in, by design. Quoting CryoThrust directly: "`updateJobState` is `synchronized` on the same monitor, so moving the not-started check under it changes the lock ordering, not just where a condition is evaluated — that deserves its own review rather than riding along with a test-and-doc change." Filed as a follow-up rather than in this PR; I agree that's reasonable since the race predates this PR. **F3** — partially in: the ordinal-guard test in `JobStatusTest` now has the guidance comment you asked for (append-only, don't just refresh the expected list). The direct `PhysicalPlan`-level behavioral test is still open, deferred to ride along with the F2 follow-up since there's no `PhysicalPlan` test class yet to build it on. **F4** — done. `NOT_STARTED_STATUSES` now has Javadoc tying it to the state machine, duplicated inline comments at both call sites are gone. Nothing here changes my Approve; happy to compare notes once you've verified the diff directly. -- 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]
