SEZ9 commented on PR #12511: URL: https://github.com/apache/seatunnel/pull/12511#issuecomment-6009155158
@Vivek1106-04 thanks for the detailed follow-up on `9143d479c` / `a691fb195` and the rebase onto `af80a6704`. **Issue 1** – the lock-holding variant is the right pin. Keeping a real drain in flight means the gate is never set without a savepoint request behind it, which is a cleaner failure mode than a reflection-set flag, and the red run with `|| savepointDraining` removed (`expected: <0> but was: <1>`) is exactly the regression signal I wanted. Agree with @Rangsh that the #12493 port can stay as a comment. **Issue 2** – gating the re-interrupt on `ForkJoinWorkerThread` plus the two tests (own thread keeps the flag, pool worker clears it) covers both sides. Good. **Issue 3** – thanks for actually running it rather than reasoning about it. `CheckpointErrorRestoreEndTest#testCancelDuringPipelineRestoreWaitEndsTheJobCanceled` going `CANCELED` vs `FAILED` on `91001c42c` confirms the regression, and matching `dev`'s outcome is the right call. One ask: you describe the non-cancel branch as "ends in the state it failed with and keeps its error" – is there a test that pins that branch as well, or only the `CANCELING` one? If not, a sibling test for the failed-state path would be good to have. **Issue 4** – noted the `forceStop` follow-up in #12622 / #12623; keeping that out of this PR is fine. Two more things: 1. Your first comment appears to cut off mid-sentence at "The savepoint case is unchanged, and the cancel…". Could you post the rest? I don't want to guess at what was covered for the remaining points. 2. On the `engine-v2-it` (JDK 11) failure in `BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure` – your reasoning (no savepoint taken, same `only observed 2` failure on `dev` in #12313 and on #12623) is convincing that it's unrelated. Please ping once the re-run has finished so I can confirm the result before approving. Once those two are in I'll do the final pass. <!-- streview-comment:1556 --> -- 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]
