SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5594890116
Thanks @DanielLeens — appreciate the careful re-trace against `303a7574890b`, and I agree with your reading on every point. On the "second read" (F1): confirmed. `cancelJob()` at `PhysicalPlan.java:212` calls `runningJobStateIMap.get(jobId)` again even though `getJobStatus()` (`PhysicalPlan.java:359-361`) is the same lookup and `jobStatus` was already captured at line 204 and proven non-null by the `isEndState()` check at line 205. `stopJob()` at line 259 correctly branches on the local. So yes: two reads, two possible snapshots, and an asymmetry between the two methods. On the null-fallthrough: also confirmed — `EnumSet.contains(null)` returning `false` means a concurrently cleared entry now falls quietly into the `CANCELING` path instead of surfacing, which is not what a pure ordinal-check replacement should do. To answer your two asks directly: 1. Yes — the fix should be to have `cancelJob()` evaluate `NOT_STARTED_STATUSES.contains(jobStatus)` off the line-204 local, matching `stopJob()`. This is not done yet on the current head; it remains blocking. 2. I don't see any reason the null-tolerant second read at line 212 would be intentionally desired over the local. Unless the author states one, the code change (not a PR-description callout) is the right resolution. If the author does want to keep it, it must be explicitly called out in the PR description. On F2 (check-then-act outside the `PhysicalPlan` monitor while `startJob()`/`stateProcess()`/`updateJobState()` are `synchronized`): agreed it predates this PR and isn't a blocker on its own. Since this branch is already being rewritten, I'd prefer folding the not-started check into the same monitor here, but a follow-up PR is acceptable if the author wants to keep this change minimal — just say which in the thread. On F3/F4: still open, still non-blocking. Concretely: Javadoc on `NOT_STARTED_STATUSES` tying it to the state machine (and dropping the duplicated inline lists at the two call sites), plus something in `JobStatusTest.java` — or a note in the guard test — that exercises the `NOT_STARTED_STATUSES` decision path and tells a future contributor what to do when the enum-table guard fails. Remaining asks, in order: - **Blocking:** `cancelJob()` uses the `jobStatus` local at line 212. - Author to state whether F2 is addressed here or deferred to a follow-up. - F3/F4 doc/test cleanups, non-blocking. Happy to re-review as soon as the F1 fix lands. <!-- streview-comment:910 --> -- 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]
