StephanEwen commented on issue #8410: [FLINK-11159] Allow configuration whether to fall back to savepoints for restore URL: https://github.com/apache/flink/pull/8410#issuecomment-505108965 This PR unfortunately changes a few sensitive core components (CheckpointCoordinator and CompletedCheckpointStore) and introduces a new configuration while pretty much violating all best practices and code style of Flink :-( Just some examples from a quick look: - constant fields should be final in class - exception rethrown as RuntimeExceptions (when signature even allows exceptions) - mockito for testing - operational config in ExecutionConfig rather than in flink-conf.yaml - bad logging `LOG.error("Method getAllCheckpoints caused exception : ", e);` - default method used to misuse interface as abstract class - interface level logger instance (side effect of the above) - and various other things in the details... I am inclined to push for reverting this commit and reopen the issue for another implementation attempt. The amount of code issues are too large for a patchup here, in my opinion. Also, as a personal take: I think we need to put an emphasis on setting good examples for the community to improve the code quality in Flink. If we don't set good examples and gradually improve the code quality, Flink will be harder and harder to maintain in the future.
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
