echauchot commented on pull request #13040: URL: https://github.com/apache/flink/pull/13040#issuecomment-685570227
> Thanks for the update @echauchot > > I see there are some git conflicts now, can you rebase your branch to the latest master? Besides, there are some checkstyle violations (CI is still failing). > sure > > 1. 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 > > Ah, you mean calling increment in the executor thread not in the other thread (See #13040 (comment))? > > No, I mean there are > > 1. cyclic dependency (coordinator <-> cleaner through a callback) which makes us use non-final volatile > 2. dependency on "different levels" (ZKStore and cleaner) which makes us pass the cleaner while building execution graph. > > Let me illustrate both the problem and a possible solution with a diagram: >  > ok thanks for the diagram, I see your point, I missed this dependency cycle. Such a class diagram on the right would require to move asyncDiscardCheckpointAndCountCheckpoints logic to the checkpoint class itself because the cleaning counter needs to be incremented before the CP discard submission (before try/catch) and the cleaning finished call and decrement need to be done in the finally. That means that the checkpoint cleaner will be responsible only for updating the counter. > > 1. it is just a delegate > > Isn't the UTest you want the test that checkpoint request is triggered after deletion completes ? > > I don't it's "just a delegate" because it contains this counter and, more importantly a callback. I think these should be covered by tests. And as the whole feature involves multiple components, I think we need an integration test. ok > > 1. `PendingCheckpoint.executor` is not used anymore so it can be deleted > sure > Thanks for letting know about your timing. Have a good time :) ---------------------------------------------------------------- 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]
