SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5578107264
Thanks @DanielLeens — agreed on all counts, and thanks for re-verifying against `303a7574890b`. Issue 1 (PR12156-F1): yes, that's exactly the concern. Beyond the asymmetry with `stopJob()`, the second `runningJobStateIMap` read means `cancelJob()` can evaluate a different snapshot than the `jobStatus` local it already holds, and the `EnumSet.contains(null)` → `CANCELING` fallthrough is a quiet behavior change for a PR described as behavior-preserving. Aligned that this is the one blocking item: `cancelJob()` should check the already-fetched `jobStatus` local, as `stopJob()` does, so both methods decide off a single snapshot. If the null-tolerant behavior is intended, it should be called out in the PR description rather than arrive as a side effect. Issue 2 (PR12156-F2): agreed it predates this PR and I wouldn't block on it alone. Since this branch is being rewritten anyway, the cheapest fix is to perform the not-started check and `updateJobState(JobStatus.CANCELED)` under the same `PhysicalPlan` monitor that `startJob()`/`stateProcess()`/`updateJobState()` already use, so the check-then-act can't interleave with a transition to SCHEDULED/RUNNING. A follow-up PR is fine if the author prefers to keep this one minimal. Issues 3 and 4 (PR12156-F3 / PR12156-F4) remain non-blocking: a short Javadoc on `NOT_STARTED_STATUSES` tying it to the state machine (and dropping the duplicated inline lists at the two call sites), a comment in `JobStatusTest` explaining what to do when the order-pinning test fails, and ideally one test that exercises the `NOT_STARTED_STATUSES` decision in `PhysicalPlan` itself. Concrete asks to unblock: (1) switch `cancelJob()` to the `jobStatus` local; (2) update the PR description if any null-path behavior change is intentional. Happy to re-review once that lands. <!-- streview-comment:882 --> -- 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]
