SEZ9 commented on PR #12129: URL: https://github.com/apache/seatunnel/pull/12129#issuecomment-5724060478
Thanks for the update. A quick follow-up on the earlier review points against head `408f1e0` — I can't see the diff for a couple of these from the thread alone, so a short confirmation on each would help: 1. **Unmodifiable `NOT_YET_STARTED_STATES`** — is the `EnumSet` now wrapped in `Collections.unmodifiableSet`? 2. **`SCHEDULED` exclusion Javadoc** — does the Javadoc on `NOT_YET_STARTED_STATES` explain that `SCHEDULED` is excluded because pipelines are already dispatched by then, so only the `CANCELING` branch of `stateProcess()` may transition out of it? That's the reasoning I'd like captured so nobody adds `SCHEDULED` to the set later. 3. **Null `JobStatus` in `cancelJob()`/`stopJob()`** — with the `EnumSet.contains(...)` check, does a null status now fail fast (NPE or explicit check), or does it fall through to the `CANCELING` path? Either way, please note which behavior is intended. 4. **Pinning test strength** — does `testOrdinalTableIsPinned()` assert both the total count of `JobStatus.values()` and the ordinal at each index, so a failure names the exact drifted constant rather than just "order changed"? 5. **Test Javadoc scope** — please make sure the pinning-test Javadoc doesn't claim more than it delivers: it protects same-build code from accidental reordering, but a mixed-version Zeta cluster (rolling upgrade) can still hit `ArrayIndexOutOfBoundsException` at the `JobStatus.values()[ordinal]` decode sites. A one-line caveat is enough. 6. **Wording** — there was a confusing sentence in that same test Javadoc; please double-check it reads cleanly after the edits above. The core change — an explicit set instead of `ordinal() <= PENDING.ordinal()` — is the right fix and is behavior-neutral on the current enum order. Once the points above are confirmed I'm happy with it. <!-- streview-comment:1124 --> -- 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]
