zhuzhurk commented on PR #23942: URL: https://github.com/apache/flink/pull/23942#issuecomment-1863877534
> Hi @zhuzhurk , thanks a lot for the comment in advance. The current PR is still missing some tests. I originally planned to develop all the tests and then find you and @JunRuiLee . > > > How about to add an internal method `RestartStrategies#exponentialDelayRestart(...)` which accepts the attempts param and modify `RestartStrategies#fromConfiguration(...)` accordingly? The change can be much simpler, which I think is better for the deprecated code path. @JunRuiLee is currently working on a common solution to pass all job configuration to JM and use it to create restartStrategy/stateBackend/etc. So I prefer to not solve it case by case which will lead to unnecessary conflicts. > > I also consider the solution you mentioned, which is really simple. The solution of current PR is using the `Configuration` to save the `restartStrategy options` inside of ExecutionConfig instead of `RestartStrategyConfiguration` object. > > For the convenience of comparison, I use SolutionA and SolutionB for them: > > * SolutionA: Using the `RestartStrategyConfiguration` object to save `restartStrategy options` > * SolutionB: Using the `Configuration` to save `restartStrategy options` > > I prefer SolutionB because some reasons: > > * `RestartStrategyConfiguration` and all sub-classes are the inner class of `RestartStrategies`. And `RestartStrategies` has been Deprecated. So we can consider `RestartStrategyConfiguration` and all sub-classes have been Depreacated. > > * SolutionA still uses dereacated classes, they cannot be removed it in 2.0. > * SolutionB doesn't use them, we can remove these classes in 2.0 directly. > * If we add other options in the future, SolutionA still needs to update these classes. > * It is more intuitive to use ConfigOptions to store all Options to Configuration. > > * We recommend users using `ConfigOptions` or key name to using the restart strategy. > * If flink developers using them in flink code directly, it's easy to maintain. > * The change of SolutionA is simpler than SolutionB, because current master branch code is SolutionA. > > * This PR(SolutionB) has some of code to compatible with `RestartStrategyConfiguration`, that's why this change is big. > * Actually, most of changes are unit tests, I need to test whether SolutionB compatible with `RestartStrategyConfiguration`. > * I believe our code will be clearer if we remove `RestartStrategies` and all classes related to `RestartStrategyConfiguration` in 2.0. > > After this PR, I believe SolutionB is very easy to add new options. > > WDYT? Looking forward to your suggestion, thanks! @JunRuiLee is working on a common solution which uses configuration to save restartStrategy options, as well as other config options, but the implementation is different, e.g. do not create restart strategy configuration first and later convert it to config option. This means that the benefits of solution A can be covered by the common solution, while solution B will add extra complexity and possible conflicts. -- 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]
