SEZ9 commented on PR #12129: URL: https://github.com/apache/seatunnel/pull/12129#issuecomment-5611598112
Thanks for the full from-scratch re-review of 5987eaf, @DanielLeens — much appreciated. On PR12129-F1 (null-state handling in `cancelJob()`/`stopJob()`): agreed with your verification. Both methods read `jobStatus = getJobStatus()` and call `jobStatus.isEndState()` before ever reaching `NOT_YET_STARTED_STATES.contains(...)`, and `getJobStatus()` is the same `runningJobStateIMap.get(jobId)` read, so a null entry NPEs on the `isEndState()` line rather than falling through to the CANCELING path. Glad the added comment reads correctly to you; I'll consider F1 closed unless you see something further. One thing I need from you: your review is marked CHANGES_REQUESTED, but the body I can see ends mid-sentence at the `stopJob()` verification, so I can't tell which of the remaining points (F2 Javadoc on why `SCHEDULED` is excluded, F3 count/per-index ordinal assertions in `testOrdinalTableIsPinned`, F4 test Javadoc overstating rolling-upgrade protection, F5 `Collections.unmodifiableSet` wrap, F6 the confusing sentence in the test Javadoc) you consider resolved by 5987eaf versus still blocking. Could you post the rest of the review, or just list the finding IDs that are still open? I'll turn those around promptly and won't touch anything outside that list. <!-- streview-comment:922 --> -- 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]
