DanielLeens commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5696044850
@CryoThrust @SEZ9 thanks both — I checked this directly against the current
head (`4c25d9a62c68`, which is also the commit my own Approve above is pinned
to), so nothing here changes my conclusion, just confirming independently:
**F1 — confirmed.** `PhysicalPlan.java:220` in `cancelJob()`:
```java
if (NOT_STARTED_STATUSES.contains(jobStatus)) {
```
No `runningJobStateIMap.get(jobId)` on this path anymore; the decision
consumes the same `jobStatus` local `isEndState()` already validated, matching
`stopJob()`'s pattern at the other call site.
**F2 — confirmed filed.** #12342 is open and correctly scoped: pre-existing
check-then-act race, not a regression from this PR, with the lock-ordering
concern (`updateJobState`'s monitor vs. `startJob()`/`stateProcess()`) spelled
out as the reason it needs its own review.
**F3 — confirmed, both halves.** `StateTransitionCleanupTest.java` now has
`testCancelOnNotStartedJobGoesStraightToCanceled` /
`testCancelOnRunningJobGoesThroughCanceling`, with an honest scope comment that
they pin the `NOT_STARTED_STATUSES` classification, not F1's single-snapshot
property — that's the right amount of claim, not overclaiming coverage.
`JobStatusTest`'s ordinal guard carries the append-only / wire-compatibility
guidance.
**F4 — confirmed.** `NOT_STARTED_STATUSES` has the state-machine Javadoc,
and the two call sites are down to one-line comments instead of duplicated
lists.
All four items check out on the diff. My Approve on `4c25d9a62c68` stands —
no further action needed from me here.
--
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]
