SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-6183754719
Thanks for the follow-up, @CryoThrust. Where the previous points stand at head 0cd5d8544e4: - **F1** – Your notes describe both `cancelJob()` and `stopJob()` branching on the single captured `jobStatus` local via `NOT_STARTED_STATUSES.contains(jobStatus)`, with the second IMap read in `cancelJob()` removed. I'd like to confirm that against the code rather than the description; could you point me at the hunk (or quote the two `if` lines at head)? Then I can close this out. - **F2** – Deferred. You've kept the follow-up separate and referenced it from the `NOT_STARTED_STATUSES` Javadoc, and this PR stays behavior-neutral. That works for me. - **F3** – The guidance in `JobStatusTest.testOrdinalOrderIsStableForInternalRpcCompatibility()` (append new statuses, do not reorder or simply refresh the expectation) covers the second half of this finding. The first half still looks open: since 0cd5d8544 is comments-only, is there a test that exercises the `NOT_STARTED_STATUSES` decision in `PhysicalPlan`? If so, please point me to it; if not, let's either add one here or agree to track it with the follow-up. - **F4** – The Javadoc on `NOT_STARTED_STATUSES` is in place per 0cd5d8544. Were the inline comments at the two call sites that duplicated the set contents also trimmed? A quick confirmation is enough. The other remaining item is a green CI run for 0cd5d8544e4. I understand the fork Actions pause is outside your control; please link the run once workflows can execute again. <!-- streview-comment:1772 --> -- 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]
