SEZ9 commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5691019646
@CryoThrust thanks for the point-by-point reply.
**F1 — looks right.** The line you quoted at `PhysicalPlan.java:220` (`if
(NOT_STARTED_STATUSES.contains(jobStatus))` consuming the same local the
`isEndState()` guard validated) is exactly what F1 asked for, and I agree the
null-state path is pre-existing and out of scope here. I'll close F1 once I've
confirmed in the `cancelJob()` hunk at
`4c25d9a62c68f9a7e13e404c1a81b3a3c56886de` that the second
`runningJobStateIMap.get(jobId)` read is gone from that path.
**F2 — accepted as a follow-up.** Agreed that moving the not-started check
under the `updateJobState` monitor changes lock ordering with
`startJob()`/`stateProcess()` and deserves its own review. One ask: please open
the follow-up issue (or link it if it already exists) and drop the link here so
the race isn't lost once this merges. Your comment also appears to have been
cut off mid-F2 ("I did not assert the race is unreachable in p"), so if there
was more after that point, please repost it.
**F3 — first half resolved, second half to verify.** The new tests in
`StateTransitionCleanupTest.java:102-142` in `4c25d9a62`
(`testCancelOnNotStartedJobGoesStraightToCanceled` /
`testCancelOnRunningJobGoesThroughCanceling`) drive `cancelJob()` from
`PENDING` and `RUNNING` and assert the resulting state, which directly
exercises the `NOT_STARTED_STATUSES` decision. What remains is the guard test
in `JobStatusTest.java`: I'll check whether it carries a comment or assertion
message telling a contributor what to do when it fails (add the new status to
`NOT_STARTED_STATUSES` or to the expected table, and check
wire/`stateTimestamps` compatibility). If it doesn't, please add one.
**F4 — will verify against the diff.** You mentioned this landed in
`bf6870507`; I'll confirm in `PhysicalPlan.java` that `NOT_STARTED_STATUSES`
now has Javadoc tying it to the state machine and that the two call-site
comments no longer duplicate the set contents.
Once the F2 follow-up link is posted and the F1/F3/F4 checks come back
clean, I expect to approve.
<!-- streview-comment:1075 -->
--
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]
