DanielLeens commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5569389084
Thanks, @SEZ9 — good catch on Issue 1, and I've verified it against the current head (`303a7574890b`): `cancelJob()` at `PhysicalPlan.java:212` reads `NOT_STARTED_STATUSES.contains(runningJobStateIMap.get(jobId))` directly from the IMap instead of reusing the `jobStatus` local already fetched at line 204, while `stopJob()` at line 259 correctly reuses that local. That's a real asymmetry I missed — I verified semantic equivalence for the *values* `NOT_STARTED_STATUSES` contains against the old ordinal check, but didn't catch that the two call sites now diverge in how they source the status to check. You're also right that the null-handling silently changed on that path: the old cast would NPE on a concurrently-removed IMap entry, `EnumSet.contains(null)` is `false` and now falls through to `CANCELING` instead — a genuine, if narrow, behavior change on what was pitched as a pure refactor. Issue 2 is fair too — I confirmed `startJob()`/`stateProcess()`/`updateJobState()` are all `synchronized` on `PhysicalPlan` while `cancelJob()`/`stopJob()` are not, so the check-then-act here isn't atomic with a concurrent state transition. That race predates this PR (the old ordinal check had the same gap), so I wouldn't block solely on it, but since the PR is already rewriting this exact branch, it's a reasonable place to close it in the same pass. Revising my conclusion from "ready to merge after fixes" (documentation nit only) to blocking on Issue 1 — please have `cancelJob()` use the already-fetched `jobStatus` local like `stopJob()` does, so both methods evaluate a single snapshot. Issue 2 and the doc/test asks (Issues 3-4) are good non-blocking follow-ups. Happy to re-review once Issue 1 is addressed. -- 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]
