SEZ9 commented on issue #12124: URL: https://github.com/apache/seatunnel/issues/12124#issuecomment-5564710508
Thanks @CryoThrust for picking this up and for keeping the scope tight — a pinned JobStatus ordinal compatibility test plus replacing the ordinal range checks in PhysicalPlan with explicit state sets is exactly what this task calls for, and leaving the internal RPC wire format untouched is the right call. Nobody else has claimed this, so it's yours. A few things I'd like to see covered in the PR you mentioned (#12156) so we can review it efficiently: 1. **Ordinal guard test**: please make sure it asserts the full ordinal table for every JobStatus value (not just the pre-start ones), with a clear failure message explaining that reordering the enum breaks internal RPC compatibility. That way a future contributor inserting a new state in the middle gets a direct hint instead of a cryptic mismatch. 2. **Explicit state sets in PhysicalPlan**: for each ordinal comparison you replace, it would help to note in the PR description (or a short code comment) which states the original range check covered, so reviewers can confirm the membership set is semantically identical and no state silently fell in or out. 3. **Cancellation/stop tests**: you mentioned adding focused tests for these paths — please include cases for a job that is cancelled before it starts and one that is cancelled after it has started, since those are the two branches the pre-start membership check distinguishes. Once those are in place, ping me on the PR and I'll take a look. Thanks again! <!-- streview-comment:870 --> -- 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]
