rmetzger commented on pull request #14948: URL: https://github.com/apache/flink/pull/14948#issuecomment-797500629
@tillrohrmann Thanks a lot for your review. I haven't responded to all comments, but they were definitely helpful pushing my thinking into the right direction. I'm sorry that we have to go through so many iterations with this. The latest version is introducing an intermediate layer between StopWithSavepoint and StopWithSavepointOperationHandlerImpl, called StopWithSavepointOperationManagerForAdaptiveScheduler. We could move this handler into the StopWithSavepoint class (except for the handleGlobalFailure method), as you can see from the dependencies and methods. I didn't do it yet to first collect some feedback on the overall approach. I am aware that the test coverage is not 100% yet, but I first want approval on the approach. I'm 90% happy with wrapping the stop with savepoint logic of SchedulerBase. There are some cases now that we need to cover that wouldn't exist if we wouldn't reuse the logic. Maybe your happiness-ratio is lower, or you prefer 100% happiness, then I'm happy to reimplement a stop with savepoint logic for the StopWithSavepoint state. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
