SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-6051250812
Thanks for the detailed CI triage, @CryoThrust — the breakdown of run `35681243175` (connector e2e `startUp` container-fetch failures, with `unit-test` and `engine-v2-it` green) and the follow-up on `37306887373` narrowing to `TestFilterRowKindIT.testFilterRowKindMultiTable` is convincing, and the rebase onto `760c33a94` with head `d59541754` should settle the Assert-sink part. Agreed that nothing in those failures touches `PhysicalPlan` or `JobStatus`; I'll wait for the fresh run on `d59541754` to confirm. That said, the two CI comments don't address the review findings themselves, and those are still what blocks approval from my side: 1. **F1 (cancelJob() second IMap read / null-state path):** please switch the not-started check in `cancelJob()` to the already-captured `jobStatus` local, mirroring `stopJob()`, so we don't do a second unsynchronized read and don't silently alter the null-state behaviour. If you believe the current form is intentional, please explain why in the thread. 2. **F2 (check-then-act outside the PhysicalPlan monitor):** the not-started check followed by `updateJobState(CANCELED)` in `cancelJob()`/`stopJob()` can still race with `startJob()`/`stateProcess()` moving the job to SCHEDULED/RUNNING. Please either move that check-and-transition under the same synchronization the state machine uses, or make the transition a conditional (compare-and-set style) update and say which you chose. 3. **F3 (tests):** `JobStatusTest` pins the enum table, but nothing exercises the `NOT_STARTED_STATUSES` decision in `PhysicalPlan`. A small test that drives `cancelJob()`/`stopJob()` on a not-started job (and ideally one for the race in F2) would cover it; please also add a short message/comment to the guard test telling a future contributor what to do when it fails. 4. **F4 (docs):** add a Javadoc on `NOT_STARTED_STATUSES` tying it to the state machine, and drop the inline comments at the two call sites that now duplicate the set contents. Once those four are pushed (and the run on `d59541754` is green), I'm happy to take another look. <!-- streview-comment:1593 --> -- 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]
