DanielLeens commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5805501672
Thanks @SEZ9 -- good news is all three of these are already confirmed on
this exact head (449b51782), from my own consolidated re-review posted a few
hours before your comment, so @CryoThrust doesn't need to redo the legwork:
1. **PR12156-F3.**
`JobStatusTest#testOrdinalOrderIsStableForInternalRpcCompatibility` already
carries the "what to do when it fails" Javadoc (move the new status to the end
of `JobStatus` rather than reordering, handle the compatibility impact
explicitly rather than just refreshing the expected list). And it's not only
the enum-table test:
`StateTransitionCleanupTest#testCancelOnNotStartedJobGoesStraightToCanceled`
and `#testCancelOnRunningJobGoesThroughCanceling` directly exercise the
`NOT_STARTED_STATUSES.contains(jobStatus)` branch in
`PhysicalPlan.cancelJob()`/`stopJob()` itself (the file even has a comment at
that pair of tests stating they pin the `NOT_STARTED_STATUSES` classification
in `PhysicalPlan`).
2. **PR12156-F2.** Position is unchanged: the check-then-act outside the
`PhysicalPlan` monitor is a pre-existing race, not introduced by this PR, and
it's filed as a properly scoped non-blocking follow-up in #12342 rather than
fixed here.
3. **PR12156-F4.** `NOT_STARTED_STATUSES` has a Javadoc at its declaration
explaining what it means and why it's an explicit set rather than an ordinal
range. The two call sites in `cancelJob()`/`stopJob()` carry a single-line
behavioral note ("Not started yet: no running work to drain, so go straight to
CANCELLED") rather than re-listing the set's members, so they're not
duplicating the constant's contents.
Nothing on the diff has changed since my approval on this head, so my "ready
to merge" conclusion stands. No action needed from me beyond this confirmation.
--
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]
