DanielLeens commented on PR #12129: URL: https://github.com/apache/seatunnel/pull/12129#issuecomment-5726030467
@SEZ9 Good news — all six of these are already confirmed against the current head (`408f1e0`) in my APPROVED review posted a few hours before your comment. Quick recap so it's easy to check off against the source: 1. **Unmodifiable `NOT_YET_STARTED_STATES`** — yes, `EnumSet.of(INITIALIZING, CREATED, PENDING)` is wrapped in `Collections.unmodifiableSet`. 2. **`SCHEDULED` exclusion Javadoc** — present, and it documents that `SCHEDULED` pipelines are already dispatched to workers, so only the `CANCELING` branch of `stateProcess()` (which calls `SubPlan.cancelPipeline()`) may transition out of that state. 3. **Null `JobStatus` in `cancelJob()`/`stopJob()`** — fails fast, doesn't fall through. `getJobStatus()` does the same `runningJobStateIMap.get(jobId)` read, and `jobStatus.isEndState()` two lines above the `contains(...)` check already dereferences it, so a null entry NPEs before `contains(...)` is ever reached. An inline comment documents this. 4. **Pinning test strength** — `testOrdinalTableIsPinned()` asserts `expected.length == actual.length` plus a per-index loop with a named failure message identifying the exact drifted constant. 5. **Test Javadoc scope** — reworded to state explicitly that this guard is same-build-only, and that a rolling-upgrade mixed-version cluster can still hit `ArrayIndexOutOfBoundsException` at the `JobStatus.values()[ordinal]` decode sites. 6. **Wording cleanup** — done as part of that same Javadoc rewrite. I walked through all six in the round-2 review of `5987eaf`, then re-verified each independently against the current source again in yesterday's APPROVED review — not just taken on trust. The only thing outstanding at this point is CI, not code: the real run on `408f1e0` had two unrelated failures (`all-connectors-it-2`/`it-6`) caused by a stale `minio/minio` Docker Hub image reference on this branch's fork point, already fixed on `dev` via #12287. Syncing this branch with `dev` should clear those. Otherwise this is approved and ready from my side. -- 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]
