SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5707392456
@CryoThrust thanks for the detailed walkthrough. A few things before I close these out on my side: **F1** — Deciding on the `jobStatus` local that `isEndState()` already validated at `PhysicalPlan.java:220` is the shape I asked for. The finding also covered the null-state path, though: what does `cancelJob()` do now when `jobStatus` is null? I'd like that spelled out (ideally with a test) before I mark F1 resolved. Separately, I can't tell from the description how `4c25d9a62` and `4f5a779a1` relate — you describe `4c25d9a62` as the ancestor of `4f5a779a1` but also say `4f5a779a1` is on another PR's branch. Could you confirm the exact head commit of this PR? **F2** — #12342 as the follow-up works for me; keeping the lock-ordering change out of this PR is the right call. Thanks for supplying the full sentence. **F3** — The scope comment on the new tests (`StateTransitionCleanupTest.java:102-142`) claiming only the `NOT_STARTED_STATUSES` classification is the right level of claim. Since a quiescent test can't distinguish the double read from the single snapshot, one deterministic option is a mock `runningJobStateIMap` that returns different values on successive reads — optional, but it would pin F1's property here rather than deferring it. The `JobStatusTest` guidance is the append-only / wire-compatibility note I wanted. **F4** — Sounds right. Could you confirm `bf6870507` is included in this PR's head? I'll do a final pass on the actual diff once the head commit is clear. <!-- streview-comment:1104 --> -- 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]
