becketqin commented on pull request #13044: URL: https://github.com/apache/flink/pull/13044#issuecomment-688413085
@pnowojski Thanks for the review. I updated the patch. Some clarifications: 1. Regarding the `checkState()` calls, I still feel a little weird to have that just to verify our own code is correct. So I just put a comment there. 2. I removed the mockito usages in `CheckpointCoordinatorTest` class and replaced them with real classes. These classes somewhat look like our own version of mockito, though. So I am not sure if the `no-mock` rule is something beneficial in all cases. Or should we still leverage that when it helps reduce the verbosity. 3. The abbreviated variable names are replaced with camel format. Personally I feel the abbreviated names are less verbose and thus more readable, but I don't have a strong opinion on that. BTW, since this ticket is a blocker for 1.11.2 release, please feel free to ping me if you have further comments. Thanks. ---------------------------------------------------------------- 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]
