SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5806532452
Following up on my earlier review against the current head `449b51782` (`449b517823f5dd4d231634eea32a52d8e5f191dc`). Per the thread, the only change since `2b0624991` is a Javadoc addition in `StateTransitionCleanupTest.java`, so I'm keeping the same scope as before. **PR12156-F1 (second IMap read in `cancelJob()`)** – The thread says `cancelJob()` now reuses the already-captured `jobStatus` local the way `stopJob()` does, with the null-status path pinned by a test. That is exactly what I asked for. Could you point me at the `cancelJob()` hunk in `PhysicalPlan.java` on `449b51782` so I can confirm it directly? **PR12156-F2 (check-then-act outside the `PhysicalPlan` monitor)** – Agreed this is pre-existing and not introduced by the ordinal-to-`EnumSet` change, so I'm fine treating it as a non-blocking follow-up. If a follow-up has been filed, please add its link to the PR description so the deferred race is discoverable from here. **PR12156-F3 (test coverage / guard-test guidance)** – The thread describes the "what to do when this fails" Javadoc on `JobStatusTest#testOrdinalOrderIsStableForInternalRpcCompatibility` and the two `StateTransitionCleanupTest` cases exercising the `NOT_STARTED_STATUSES.contains(jobStatus)` branch. That matches what I asked for. Please point me at those hunks on `449b51782` and I'll close this out; the added note that the cleared-status test pins pre-existing behaviour rather than a design contract is a good touch. **PR12156-F4 (Javadoc on `NOT_STARTED_STATUSES` / duplicated call-site comments)** – Javadoc at the declaration plus a single behavioural line at each call site is the shape I wanted. Same ask: a pointer to the `PhysicalPlan.java` hunk on `449b51782` and I'll mark it resolved. Summary of remaining asks before I approve: (1) the `cancelJob()` hunk for F1, (2) the follow-up link for F2 in the description, and (3) the test and `PhysicalPlan.java` hunks for F3/F4. Nothing else from my previous scope is outstanding. <!-- streview-comment:1272 --> -- 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]
