SEZ9 commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5825967412
Thanks for the follow-up on the review points — going through them against
head `449b51782`:
**F1 (second IMap read in `cancelJob()`)** — The `cancelJob()` hunk quoted
above now branches on the `jobStatus` local captured at the top of the method
(the same one already validated by `isEndState()`), with no second
`runningJobStateIMap.get(jobId)` on that path. That matches what `stopJob()`
does, so I consider this resolved.
**F2 (check-then-act outside the `PhysicalPlan` monitor)** — I'm fine with
treating this as a pre-existing race and deferring it to #12342 rather than
fixing it here. One thing is still outstanding, though: the PR description
(`Motivation` / `Changes` / `Verification` / `Fixes #12124`) doesn't reference
#12342 yet. Please add a line linking the follow-up so the deferred race stays
discoverable from this PR.
**F3 (test coverage / guard explanation)** — The Javadoc on
`testOrdinalOrderIsStableForInternalRpcCompatibility` now tells a future
contributor exactly what to do (append to the end of `JobStatus`, and handle
the compatibility impact explicitly rather than just refreshing the expected
list), and `testCancelOnNotStartedJobGoesStraightToCanceled` /
`testCancelOnRunningJobGoesThroughCanceling` in `StateTransitionCleanupTest`
drive `cancelJob()` through both sides of the
`NOT_STARTED_STATUSES.contains(jobStatus)` branch. That's the
`PhysicalPlan`-level coverage I was asking for — resolved.
**F4 (`NOT_STARTED_STATUSES` Javadoc / duplicated comments)** — From the
thread, the constant now has a Javadoc explaining why it's an explicit set
rather than an ordinal range, and the two call sites carry a single behavioral
note ("Not started yet: no running work to drain, so go straight to CANCELLED")
instead of re-listing the members. The second walkthrough above cut off before
reaching F4, so could you briefly confirm that Javadoc is present on this head?
If so, this one is closed as well.
Remaining asks:
1. Add the `#12342` link to the PR description.
2. Confirm the `NOT_STARTED_STATUSES` Javadoc is in place on `449b51782`.
Once those are in, I'm good to move forward with this.
<!-- streview-comment:1305 -->
--
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]
