SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-6432310603
Thanks @CryoThrust for syncing with current `dev` in `7938f302f` and for the clear conflict note — keeping the change scoped to the original three files while preserving the upstream stop/savepoint handling in `PhysicalPlan` is what I was hoping for, and the local `JobStatusTest` / `StateTransitionCleanupTest` runs are good to see. Where my earlier points stand on this head: 1. **Single `jobStatus` snapshot in `cancelJob()`** — resolved. `cancelJob()` now branches on the `jobStatus` local already validated by `isEndState()`, the second IMap read is gone, and the null path is unchanged, matching `stopJob()`. 2. **Check-then-act outside the `PhysicalPlan` monitor** — still open from my side. A concurrent `startJob()`/`stateProcess()` can move the job to SCHEDULED/RUNNING between the `NOT_STARTED_STATUSES` check and `updateJobState(CANCELED)`. If you consider this pre-existing and out of scope for this PR, please say so explicitly and I'm happy to track it separately; otherwise, does the not-started branch need to run under the same lock? 3. **Guard test comment** (low, non-blocking) — the PR pins the ordinal table and the cancel decision, so coverage is fine. A short comment on the ordinal guard test explaining that a failure means deliberately updating the expected table rather than reordering the enum would help future contributors. 4. **Javadoc on `NOT_STARTED_STATUSES`** (low, non-blocking) — a brief Javadoc tying the set to the state machine (which states mean no pipeline has been deployed yet), plus trimming the two call-site comments so they don't repeat the set contents. Items 3 and 4 don't block the merge; an answer on 2 is the main thing I'd like to settle. No need for an empty commit to poke CI. <!-- streview-comment:2068 --> -- 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]
