DanielLeens commented on PR #12129: URL: https://github.com/apache/seatunnel/pull/12129#issuecomment-5611962695
@SEZ9 To directly answer — my `CHANGES_REQUESTED` review of `5987eaf` is complete on the source of truth (I just re-pulled the raw body via the API and it runs all the way through the `## CI Diagnosis` section, no truncation). If GitHub's web UI cut it off for you, that's a rendering issue on their end, not a missing section on mine — pasting the resolution status below so we don't lose time on it either way. Status per finding, verified against the current head in that review: - **F1** (null-state handling in `cancelJob()`/`stopJob()`) — **Resolved.** Confirmed unreachable: both methods call `getJobStatus()` then `jobStatus.isEndState()` two lines before `NOT_YET_STARTED_STATES.contains(...)`, so a null map entry NPEs there first. - **F2** (Javadoc on why `SCHEDULED` is excluded) — **Resolved.** Present, and I traced it correctly against `stateProcess()`'s `CANCELING` case (`SubPlan.cancelPipeline()`). - **F3** (count + per-index ordinal assertions in `testOrdinalTableIsPinned`) — **Resolved.** Verified byte-for-byte against the real `JobStatus` enum declaration. - **F4** (Javadoc same-build-only scope statement) — **Resolved.** Accurate, matches the RPC-decode risk I traced independently. - **F5** (`Collections.unmodifiableSet` wrap) — **Resolved.** Present and correct. - **F6** (confusing sentence cleanup) — **Resolved**, folded into the F3/F4 Javadoc rewrite verification above. So all six of your items are closed as of `5987eaf`. The one thing I added on top this round is a new, **non-blocking** Issue 1 in that review: the guard test's Javadoc site-list names `GetJobStatusOperation`/`ClientJobProxy`/`JobClient` but misses two more RPC-ordinal-decode sites (`JobInfoService.java:360`, `BaseService.java:386`) and two E2E range-comparison sites of the same anti-pattern (`ClusterFaultToleranceIT.java:121-122`, `ClusterFaultToleranceTwoPipelineIT.java:264-265`). That's a recommended follow-up, not a re-open of F1-F6. The only actual blocker left is CI: the fork (`akshar27/seatunnel`) still shows zero Actions runs for `5987eaf`, and `apache/seatunnel`'s `Build` check is `ACTION_REQUIRED` — same "enable Actions on your fork" signature as before, nothing code-side to fix. -- 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]
