SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-6009585366
Thanks for the rebase onto `eb2eb1ddc` and the detailed breakdown, @CryoThrust. The connector-e2e classification for `449b51782` (container fetch/startup failures in `startUp`, nothing on the classpath of the changed engine modules) is reasonable, and I'm happy to wait for run `37306887373` on `95504b5fa` to finish rather than treat those partitions as a code regression. If it comes back with the same shape, that's sufficient from my side on the CI question. That said, CI isn't the only thing between this and a merge. The review findings from the previous pass are still open, and nothing in the thread so far says whether `95504b5fa` touched them beyond the rebase: 1. **`cancelJob()` second IMap read** — it should use the already-captured `jobStatus` local the same way `stopJob()` does, rather than performing a second unsynchronized read for the not-started check (and silently changing the null-state path). 2. **Check-then-act outside the `PhysicalPlan` monitor** — the not-started check in `cancelJob()`/`stopJob()` can race with `startJob()`/`stateProcess()` moving the job to `SCHEDULED`/`RUNNING`, so `updateJobState(CANCELED)` can land on a job that already has pipelines deploying. Please either take the monitor around the check-and-transition or explain why that window is safe. 3. **Test coverage** — `JobStatusTest` pins the enum table, but nothing exercises the actual `NOT_STARTED_STATUSES` decision in `PhysicalPlan`. Please add a test for that path, and give the guard test a short comment telling a future contributor what to do when it fails (i.e. which of the two sets to update and why). 4. **Docs/comments** — add a Javadoc on `NOT_STARTED_STATUSES` tying it to the state machine, and drop the inline comments at the two call sites that currently just duplicate the set contents. Could you confirm whether any of these are already addressed in `95504b5fa`, and if not, push the follow-up so I can review the engine changes and the fresh CI run together? Once those four are in, and the new run on `95504b5fa` is clean on the engine/unit jobs, I'm fine moving this forward regardless of the unrelated connector partitions. <!-- streview-comment:1560 --> -- 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]
