DanielLeens commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5578527719
Thanks @SEZ9 — I re-traced the "second read" concern directly against the
current head (`303a7574890b`) and it holds up exactly as you described.
`cancelJob()` (`PhysicalPlan.java:203-212`) does:
```
JobStatus jobStatus = getJobStatus(); // line 204 — read #1
if (jobStatus.isEndState()) { ... return; } // line 205 — guarantees
jobStatus is non-null here
if (NOT_STARTED_STATUSES.contains(runningJobStateIMap.get(jobId))) { //
line 212 — read #2, independent
```
and `getJobStatus()` (`PhysicalPlan.java:359-361`) is itself just `return
(JobStatus) runningJobStateIMap.get(jobId);` — so line 212 is a fresh, second
call into the same IMap rather than a reuse of the `jobStatus` local that line
205 already proved non-null. `stopJob()` (`PhysicalPlan.java:252-259`) does the
equivalent check as `NOT_STARTED_STATUSES.contains(jobStatus)`, off the single
local. So the asymmetry and the "two reads can observe two different snapshots"
issue are both confirmed as still present, and confirmed as a genuine (if
narrow) divergence from "pure refactor."
On the null-fallthrough: since `EnumSet.contains(null)` returns `false`
rather than throwing, if the IMap entry is concurrently cleared between the two
reads, `cancelJob()` now silently falls through to the normal cancellation
branch instead of surfacing anything — a quiet behavior change on a path billed
as ordinal-check replacement only. Agreed this needs either the code fix or an
explicit callout in the PR description, not both left implicit.
Concrete asks, agreed as stated: (1) `cancelJob()` should branch on the
`jobStatus` local already fetched at line 204, matching `stopJob()`'s pattern,
so both methods commit to one snapshot; (2) if there's any reason the
null-tolerant fallthrough at line 212 is intentionally desired over the local,
that has to be called out explicitly in the PR description rather than ride
along as a side effect of the refactor.
Issue 2 (the pre-existing check-then-act race against the `synchronized`
transitions in `startJob()`/`stateProcess()`/`updateJobState()`) — agreed it
predates this PR and isn't a blocker on its own; folding the not-started check
into the same monitor while this branch is already being touched is a
reasonable low-cost improvement, but a follow-up PR is fine too if the author
wants to keep this change minimal.
Issues 3/4 (Javadoc on `NOT_STARTED_STATUSES` tying it to the state machine
instead of duplicating the inline list at each call site, a comment on the
order-pinning `JobStatusTest` explaining what a failure means, and a direct
`PhysicalPlan`-level test for the `NOT_STARTED_STATUSES` decision) remain good
non-blocking follow-ups.
My conclusion stands as blocking on Issue 1 only (the `cancelJob()` snapshot
fix, or an explicit description update if the null-path change is intentional);
I'll do the full re-review as soon as that commit lands.
--
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]
