echauchot commented on pull request #13040: URL: https://github.com/apache/flink/pull/13040#issuecomment-672843805
> Thanks for the PR @echauchot => Thanks Roman for the detailed review and also for your suggestions throughout the design ! Comments inline and in the reviewed code. > > I think the changes are correct in general, I've left some comments in code. > > Besides that, I have some general remarks: > > 1. Please make sure CI build passes (see @flinkbot comments). I think you missed the license in a new file. => Yes I saw the RAT failure in Azure CI build, it did not show up using the verify maven phase when I launched the build locally, that is why I forgot it :) > 2. Cleanup commit history (see also [changes guide](https://flink.apache.org/contributing/code-style-and-quality-pull-requests.html)) > remove reverted, e.g. `de8197` > squash, e.g. `06c763` > make each commit correct, e.g. `numberOfCleaningCheckpointsSupplier.get() > maxQueuedRequests` check in `05f1cb` > probably it's easier to have just one commit? I think the changes aren't so big and are related => yes I left separate commits to ease the review (allow to isolate parts corresponding to comments in the design doc and isolate further review commits). My plan was indeed to do all the squashing at the end of the review. This can definitely be a single commit as the changes are bound to only one module and do not change the public API etc... > 3. I think we can re-structure the code so that ZKStore doesn't depend on `CheckpointCleaner`; for this, include a call to `CheckpointCleaner` into `CompletedCheckpoint.discardCallback` => how can we do that ? I can decrement the counter in the callback which is executed in the Executor but the incrementation of the counter is done before submitting the callback to the executor, so outside the callback. Do I misunderstand something ? > 4. These tests are missing: > > * unit tests for `CheckpointsCleaner` => it is just a delegate for checkpoint async tasks which are already tested in ZooKeeperCompletedCheckpointStoreTest, PendingCheckpoint. The only thing added is the counter inc/dec. I added a test only for CheckpointRequestDeciderTest to test that when threshold is exceeded, there is no CP request triggered and that there is one when the counter decreases. Isn't the UTest the second test below ? > * test that checkpoint request is triggered after deletion completes ---------------------------------------------------------------- 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]
