CryoThrust commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5663081824
Thanks both for the careful trace, and sorry for the slow turnaround. Pushed
`bf6870507`, which addresses F1.
**F1 (blocking):** `cancelJob()` now branches on the `jobStatus` local
captured at the top, matching `stopJob()`:
```java
if (NOT_STARTED_STATUSES.contains(jobStatus)) {
```
So both methods commit to the single snapshot, and the second
`runningJobStateIMap.get(jobId)` is gone. You were right that the null-tolerant
fallthrough was not intended — it was a side effect of re-reading the IMap, not
a deliberate choice, so I fixed the code rather than documenting it in the
description.
I also took the two non-blocking cleanups while the branch was open (F4 and
the cheap half of F3): the state-machine meaning now lives in a Javadoc on
`NOT_STARTED_STATUSES` and the duplicated inline list is dropped from both call
sites, and `JobStatusTest`'s ordinal guard carries a comment saying what a
failure means and why refreshing the expected list is the wrong move.
**F2 (check-then-act outside the `PhysicalPlan` monitor):** I'd rather not
fold it in here. `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. Say the word if you'd prefer it in this PR
and I'll add it; otherwise I'll file it as a follow-up so it doesn't get lost.
**F3 remainder** (a direct `PhysicalPlan`-level test for the
`NOT_STARTED_STATUSES` decision) — agreed it's worth having; there's no
`PhysicalPlan` test class today, so I'd rather add it together with the F2 work
than create a throwaway harness here.
Validation: `JobStatusTest` 2/2, and `JobMasterTest` + `SavePointTest` +
`CoordinatorServiceWithCancelPendingJobTest` 17 run / 0 failures / 1 skipped,
which covers the cancel and stop paths.
--
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]
