DanielLeens commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5852827488
@SEZ9 understood on wanting to check the diff yourself rather than take quoted snippets on trust — that's the right instinct in general. To make that easy without relying on my transcription, here are direct links to the three changed files at the exact head commit (`449b517823f5dd4d231634eea32a52d8e5f191dc`), plus the cumulative diff view: - Full diff for this head: https://github.com/apache/seatunnel/pull/12156/files - `PhysicalPlan.java` at this head: https://github.com/apache/seatunnel/blob/449b517823f5dd4d231634eea32a52d8e5f191dc/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/dag/physical/PhysicalPlan.java (the `NOT_STARTED_STATUSES` Javadoc is at lines 54-63, `cancelJob()` at 211-224, `stopJob()`'s equivalent branch around line 267) - `JobStatusTest.java` at this head: https://github.com/apache/seatunnel/blob/449b517823f5dd4d231634eea32a52d8e5f191dc/seatunnel-engine/seatunnel-engine-common/src/test/java/org/apache/seatunnel/engine/common/job/JobStatusTest.java (guard-test Javadoc at lines 41-49) - `StateTransitionCleanupTest.java` at this head: https://github.com/apache/seatunnel/blob/449b517823f5dd4d231634eea32a52d8e5f191dc/seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/dag/physical/StateTransitionCleanupTest.java (the four cases are `testCancelJobCommitsToTheJobStatusSnapshotItValidated` at line 114, `testCancelOnNotStartedJobGoesStraightToCanceled` at 140, `testCancelOnRunningJobGoesThroughCanceling` at 153, `testCancelJobOnClearedJobStatusFailsAtTheEndStateGuard` at 177) To be clear about what I did on my side, since it matters for how much weight to put on it: my confirmations in the two prior comments weren't "quoted from the thread" — I pulled `PhysicalPlan.java` and the test files directly from the repository at this exact SHA via the API each time (not from CryoThrust's description, not from my own earlier review), and pasted the literal hunks I read. So F1/F3/F4 are independently diff-verified on my end, not just restated claims. But I fully support you doing the same check yourself before signing off — no need to take my word for it, the links above should make that a direct look rather than a re-ask of the author. On F2: agreed, and that one's on @CryoThrust, not something either of us can resolve from review comments — the ask is a one-line edit to the PR description linking #12342 so the deferred check-then-act race stays discoverable after merge. That's the only outstanding item on my side as well. So, to summarize where this leaves the PR: F1/F3/F4 are resolved and independently confirmed against the live diff (by me, and hopefully now directly by you via the links above), and F2 is accepted as a non-blocking follow-up pending the description link. Nothing here changes my Approve on this head. -- 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]
