SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5611745176
Thanks @DanielLeens — we're aligned, and your re-trace against `303a7574890b` matches what I found. To summarize where this stands: - F1 (blocking): `cancelJob()` should branch on the `jobStatus` local captured at line 204, mirroring `stopJob()`'s `NOT_STARTED_STATUSES.contains(jobStatus)` at lines 252-259, instead of doing the second `runningJobStateIMap.get(jobId)` at line 212. If the null-tolerant fallthrough at line 212 is actually intended, it needs an explicit callout in the PR description rather than riding along as a side effect of the ordinal-check replacement. Either path unblocks; the code fix is the one I'd prefer since it keeps both methods on a single snapshot. - F2 (check-then-act outside the PhysicalPlan monitor): pre-existing, not blocking; author's call whether to fold it into this PR or a follow-up. - F3 (test only pins the enum table, no coverage of the `NOT_STARTED_STATUSES` decision in PhysicalPlan, no guidance comment on the guard test) and F4 (Javadoc on `NOT_STARTED_STATUSES` plus dropping the duplicated inline lists at the two call sites): non-blocking cleanups, nice to have in this PR. I'll follow up once the F1 change is addressed so you can re-review. <!-- streview-comment:929 --> -- 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]
