SEZ9 commented on PR #12511: URL: https://github.com/apache/seatunnel/pull/12511#issuecomment-6029392209
@Vivek1106-04 thanks for following up on all three points. 1. Good to hear the re-run on `a691fb195` finished green, including `engine-v2-it (11)` and the rest of the engine jobs. That matches your earlier analysis that `BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure` is not driven by this PR (no savepoint in that test, same `only observed 2` failure on `dev`), so I consider the CI question closed. 2. Understood on the truncated preview — thanks for pointing to issuecomment-5982000557 and quoting the rest of the sentence. The intended outcome is clear: a job that is `CANCELING` when the restore is abandoned ends `CANCELED`, otherwise the pipeline keeps the state and error it failed with, and the cancelled job no longer restarts a pipeline only to cancel it. That is the behaviour I expected. 3. Thanks for clarifying where the non-cancel branch is pinned. `testSavePointFailureDuringPipelineRestoreWaitEndsTheJob` in `SavePointTest` (job stays `DOING_SAVEPOINT` / pipeline stuck `RUNNING` without `abandonRestore`, so it times out) together with `testCancelDuringPipelineRestoreWaitEndsTheJobCanceled` in `CheckpointErrorRestoreEndTest` covers both branches, which answers my concern. One small ask so the rationale is easy to find later: could you add a short note in the PR description (or in a comment next to `abandonRestore`) that points to these two tests as the pins for the cancel and non-cancel branches? No code change needed beyond that from my side. <!-- streview-comment:1563 --> -- 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]
