DanielLeens commented on PR #12156:
URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5812762812
@SEZ9 happy to point you at all three directly — I pulled each hunk from the
current head (`449b51782` / `449b517823f5dd4d231634eea32a52d8e5f191dc`) rather
than from the thread history, so these are fresh confirmations, not restated
claims.
**F1 — `cancelJob()` hunk.** `PhysicalPlan.java:211-224`:
```java
public void cancelJob() {
JobStatus jobStatus = getJobStatus();
if (jobStatus.isEndState()) {
...
return;
}
if (NOT_STARTED_STATUSES.contains(jobStatus)) {
// Not started yet: no running work to drain, so go straight to
CANCELLED.
updateJobState(JobStatus.CANCELED);
jobEndFuture.complete(new JobResult(JobStatus.CANCELED));
} else {
updateJobState(JobStatus.CANCELING);
}
}
```
Line 220 consumes the `jobStatus` local captured at line 212 and already
validated by `isEndState()` — there is no second
`runningJobStateIMap.get(jobId)` on this path. Confirmed.
**F2 — one item is still genuinely open.** I checked the PR description
itself via the API rather than assuming: it currently has `Motivation` /
`Changes` / `Verification` / `Fixes #12124`, and does not mention `#12342`
anywhere. So the follow-up is filed, but the link isn't in the description yet
— that part of your ask isn't done. @CryoThrust, could you add a line linking
`#12342` so the deferred race stays discoverable from this PR?
**F3 — both halves, confirmed in the diff.**
- `JobStatusTest.java:41-49` (Javadoc directly above
`testOrdinalOrderIsStableForInternalRpcCompatibility`):
```java
/**
* The ordinal position of every status is part of the internal RPC
contract, so reordering or
* inserting a constant breaks wire compatibility with older members.
*
* <p>If this test fails, move the new status to the end of {@link
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.
*/
```
- `StateTransitionCleanupTest.java:140`
(`testCancelOnNotStartedJobGoesStraightToCanceled`, asserts `PENDING` →
`CANCELED`) and `:153` (`testCancelOnRunningJobGoesThroughCanceling`, asserts
`RUNNING` → `CANCELING`) both drive `cancelJob()` directly and assert on
`NOT_STARTED_STATUSES.contains(jobStatus)`'s two branches — this is the
`PhysicalPlan`-level coverage, not just the enum table. There's also a third
test at `:177`, `testCancelJobOnClearedJobStatusFailsAtTheEndStateGuard`, plus
`:114`'s `testCancelJobCommitsToTheJobStatusSnapshotItValidated`, which is the
single-snapshot pin we discussed earlier in the thread.
**F4 — confirmed.** `PhysicalPlan.java:54-63`:
```java
/**
* Job states that have not yet started running. Cancelling or stopping a
job in one of these
* states moves it straight to {@link JobStatus#CANCELED} instead of going
through the {@link
* JobStatus#CANCELING} transition, because there is no running work to
drain.
*
* <p>Kept as an explicit status set rather than an ordinal range so that
adding a status in the
* middle of the enum cannot silently change which states are considered
"not started".
*/
private static final Set<JobStatus> NOT_STARTED_STATUSES =
EnumSet.of(JobStatus.INITIALIZING, JobStatus.CREATED,
JobStatus.PENDING);
```
and both call sites (`:221` in `cancelJob()`, `:267` in `stopJob()`) are
down to the single-line `// Not started yet: no running work to drain, so go
straight to CANCELLED.` — no duplicated list of the set's members.
So: F1/F3/F4 all check out on the diff itself. F2 is the one loose end — the
follow-up issue exists, it just isn't linked from this PR yet. My approval on
this head stands regardless (F2 was always non-blocking), but flagging it so it
doesn't fall through the cracks before merge.
--
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]
