akshar27 commented on PR #12129:
URL: https://github.com/apache/seatunnel/pull/12129#issuecomment-5592574038
Pushed 5987eaf addressing all three items:
**1. cancelJob() null-safety** — added a comment: `jobStatus.isEndState()`
two lines above the `NOT_YET_STARTED_STATES.contains(...)` call already
dereferences the same `runningJobStateIMap.get(jobId)` entry, so a null there
throws NPE before execution ever reaches the `contains(...)` call — it's
unreachable, not silently swallowed as "not yet started."
**2. NOT_YET_STARTED_STATES** — added Javadoc explaining why `SCHEDULED` is
excluded: I traced `stateProcess()`'s `CANCELING` case, which calls
`SubPlan.cancelPipeline()` on every pipeline. If `SCHEDULED` (pipelines already
dispatched to workers by that point) were treated as "not yet started,"
`cancelJob()` would jump straight to `CANCELED` and complete the job-end future
without ever calling `cancelPipeline()` — orphaning already-dispatched pipeline
resources. Also wrapped the set in `Collections.unmodifiableSet`.
**3. JobStatusTest** — replaced `assertArrayEquals` with a total-count
assertion plus a per-ordinal loop, each with a named failure message
("JobStatus ordinal N drifted from the pinned constant"), so a drift names the
exact constant. Reworded the Javadoc to state the test's purpose up front (the
old sentence read ambiguously, as if the corruption it prevents was still
happening) and made explicit that this guard only covers same-build code — a
rolling upgrade with a reordered/resized `JobStatus` can still hit
`ArrayIndexOutOfBoundsException` or a wrong-constant decode at the RPC sites,
per @DanielLeens's traced analysis, which needs a separate name-based RPC
transport design (out of scope here).
Re-ran `spotless:apply`, `mvn compile` on the affected modules (both
succeed), and re-verified the ordinal table + `EnumSet`-vs-`ordinal()`
equivalence via the same standalone `javac`/`java` approach from the PR
description, since `mvn test` is still unreliable in my environment for the
unrelated reason described there.
GitHub Actions still isn't enabled on my fork as of this push — still
pending on my end, will ping again once it's confirmed on.
--
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]
