akshar27 opened a new pull request, #12129: URL: https://github.com/apache/seatunnel/pull/12129
### Purpose of this pull request `JobStatus`'s ordinal is relied on directly in two ways with no guard against it changing: - Transported raw over the internal RPC: `GetJobStatusOperation.java:81` sends `future.get().ordinal()`; `ClientJobProxy.java:154` and `JobClient.java:121` decode it back with `JobStatus.values()[ordinal]`. - Used to index the `stateTimestamps` array in `PhysicalPlan` (`PhysicalPlan.java:105`, `:115`, `:300`, `:309`). `PhysicalPlan.java:207` and `:254` additionally use `jobStatus.ordinal() <= JobStatus.PENDING.ordinal()` as a range check for "job hasn't started running yet" (`INITIALIZING`/`CREATED`/`PENDING`), which only works because those three happen to be the first three enum constants. This is a hardening task — there's no current failure caused by the ordering, the goal is to make the existing coupling explicit and guarded, per the issue. Changes: - Added `JobStatusTest.testOrdinalTableIsPinned`, which pins the exact `JobStatus.values()` order and fails loudly if a future change reorders or inserts a constant. - Replaced the two `ordinal() <= PENDING.ordinal()` range comparisons in `PhysicalPlan` with an explicit `NOT_YET_STARTED_STATES` `EnumSet` (`INITIALIZING`, `CREATED`, `PENDING`), so the actual intent is explicit instead of relying on enum declaration order. Per the issue, a name-based/versioned RPC transport is a separate design item and out of scope here. The `stateTimestamps` array indexing is left as-is (still ordinal-based) — it's now covered by the pinning test, so a reorder will fail the test before it can silently corrupt the array or the RPC decode. Fixes #12124 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Added `JobStatusTest.testOrdinalTableIsPinned` (see above). - `./mvnw -q spotless:apply` and `./mvnw -q -DskipTests compile -pl seatunnel-engine/seatunnel-engine-common,seatunnel-engine/seatunnel-engine-server -am` both succeed cleanly against this change. - I was not able to get `./mvnw test` to run cleanly for these modules in my local environment — a shallow clone of the reactor hits a pre-existing module-ordering issue unpacking `seatunnel-config-shade`'s vendored typesafe-config sources (reproducible on unmodified `dev`, unrelated to this change: the same "cannot find symbol" failures occur in `seatunnel-common`'s `CheckConfigUtil`/`TypesafeConfigUtils`, files this PR doesn't touch). As a substitute, I compiled `JobStatus.java` standalone (zero external dependencies) and ran a small throwaway program executing both the new pinning assertion and the old-vs-new (`ordinal() <= PENDING.ordinal()` vs. the new `EnumSet.contains`) comparison against every current `JobStatus` value — confirmed identical results for all 13 states. Happy for a maintainer to confirm `mvn test` passes in CI. ### Check list * [ ] If any new Jar binary package adding in your PR, please add License Notice according [New License Guide](https://github.com/apache/seatunnel/blob/dev/docs/en/developer/new-license.md) — N/A, no new dependencies * [ ] If necessary, please update the documentation to describe the new feature — N/A, internal hardening only, no user-facing behavior change * [ ] If necessary, please update `incompatible-changes.md` — N/A, not an incompatible change * [ ] Connector-code checklist items — N/A, this PR doesn't touch a connector -- 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]
