CryoThrust commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5676265738
@SEZ9 thanks for the detailed pass. Pushed `4c25d9a62` for the remaining F3
item; the rest was already in `bf6870507`. Point by point:
**F1 — the hunk.** `PhysicalPlan.java:220`, in `cancelJob()`:
```java
if (NOT_STARTED_STATUSES.contains(jobStatus)) {
```
The previous line in the method is still `JobStatus jobStatus =
getJobStatus();` followed by the `isEndState()` guard, so the decision now
consumes the same local that guard validated. There is no longer a
`runningJobStateIMap.get(jobId)` on this path. On the null-state question:
unchanged by this PR, as DanielLeens traced — a null `getJobStatus()` NPEs at
`isEndState()` before either branch, both before and after. What F1 removed was
the second read observing a *different non-null* status, not the null case.
**F2 — answered, not folded in.** `updateJobState` is `synchronized` on the
same monitor as `startJob()`/`stateProcess()`, so pulling the not-started check
under that monitor changes lock ordering rather than just where a condition is
evaluated. That deserves its own review instead of riding along with a
test-and-doc change, so I left it out and it should be filed as a follow-up. I
did not assert the race is unreachable in practice.
**F3(a) — done in `4c25d9a62`.** Two tests in `StateTransitionCleanupTest`
(the existing `PhysicalPlan` harness): cancel on `PENDING` reaches `CANCELED`,
cancel on `RUNNING` reaches `CANCELING`. `JobMaster` is mocked for the job-end
future and state-event reporting. 5 run / 0 failures.
One honest limitation, called out in a comment on the tests: they pin the
`NOT_STARTED_STATUSES` *classification*, not F1's single-snapshot property. I
checked by reverting `cancelJob()` to the double-read — both tests still pass,
because in a quiescent test both reads observe the same value. Guarding that
property would need a map that returns divergent values per read; I did not
want to claim coverage the tests do not have.
**F3(b) and F4** — in `bf6870507`: the ordinal guard test carries the
append-only guidance, and `NOT_STARTED_STATUSES` has the state-machine Javadoc
with the duplicated inline comments dropped.
Happy to file the F2 follow-up with the PhysicalPlan-level test scaffold now
in place, if that is the preferred route.
--
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]
