akalash commented on pull request #18852: URL: https://github.com/apache/flink/pull/18852#issuecomment-1050968264
@1996fanrui, we discussed with @pnowojski about this task a little. As result I have some important comments: - I asked you moving logging from `CheckpointCoordinator` to `CheckpointFailureManager` but actually, it makes sense to do the same thing with `reportFailedCheckpoint`. So instead of reporting failed checkpoint from `PendingCheckpoint#abort` we can do it from `CheckpointFailureManager`. - I also thought about your new method `reportFailedCheckpointsWithoutInProgress` perhaps you can leave it as is since my idea about calling the statistic before the checkpoint id increment is not so easy to implement.(especially the part with reporting failed checkpoint). As result, I think it makes sense to have three commits in this PR: - Moving checkpoint location initialization(refactor commit) - Moving logging and reporting failed checkpoint to `CheckpointFailureManager`(refactor commit) - Changing the logging text and adding new logic about reporting checkpoint without pending checkpoint(bug fix commit) Sorry, that the idea is slightly changed since my first comments but I think it will be easier to implement. And also if some of my proposals is not clear or not so good for you, please, give me now. -- 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]
