CryoThrust commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5693753551
@SEZ9 thanks for the pass — all four items are addressed, and the F2
follow-up now exists.
**F1 — confirmed in the diff.** `PhysicalPlan.java:220` in `4f5a779a1`'s
ancestor `4c25d9a62`:
```java
if (NOT_STARTED_STATUSES.contains(jobStatus)) {
```
The second `runningJobStateIMap.get(jobId)` read is gone from that path; the
decision consumes the local the `isEndState()` guard validated. (Line number is
stable at `220` on the current head — `4f5a779a1` is on a different PR's branch
and does not touch this file.)
**F2 — follow-up filed: #12342.** It records the check-then-act race, why it
was kept out of this PR (moving the check under the `updateJobState` monitor
changes lock ordering with `startJob()`/`stateProcess()`, not just where a
condition is evaluated), and the ask that any fix keep the
`NOT_STARTED_STATUSES` semantics this PR establishes. Apologies for the
truncated sentence you spotted — the full text was "I did not assert the race
is unreachable in practice"; that is precisely why it needs its own review
rather than a justification here.
**F3 — both halves.** The `PhysicalPlan`-level tests are in
`StateTransitionCleanupTest.java:102-142` (cancel on `PENDING` → `CANCELED`,
cancel on `RUNNING` → `CANCELING`, `JobMaster` mocked for the job-end future
and state-event reporting). On the guard test: `JobStatusTest` now carries
> If this test fails, move the new status to the end of `JobStatus` (and
update this list) rather than reordering existing constants. When inserting one
is unavoidable, handle the compatibility impact explicitly — do not just
refresh the expected list, or every stored ordinal and every in-flight status
report silently changes meaning.
which is the append-only + wire/`stateTimestamps` guidance you asked for.
One honest limitation, also noted in a comment on the new tests: they pin the
`NOT_STARTED_STATUSES` *classification*, not F1's single-snapshot property — I
verified by reverting `cancelJob()` to the double-read that both still pass,
since a quiescent test sees the same value on both reads.
**F4 — in `bf6870507`.** `NOT_STARTED_STATUSES` carries the state-machine
Javadoc, and the two call sites are down to a one-line comment each, so the set
contents live in one place.
--
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]
