StefanRRichter commented on a change in pull request #8322: [FLINK-12364]
Introduce a CheckpointFailureManager to centralized manage checkpoint failure
URL: https://github.com/apache/flink/pull/8322#discussion_r282392201
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java
##########
@@ -115,6 +115,8 @@
private volatile ScheduledFuture<?> cancellerHandle;
+ private final CheckpointFailureManager failureManager;
Review comment:
Thinking in terms of objects and there relationships and separation, I
wonder why a pending checkpoint needs to know the failure manager. If at all, I
would rather think a failure manager should know the pending checkpoints and
manage them on failures and success. Most straight forward would be that the
coordinator has a `CheckpointFailureManager` and knows `PendingCheckpoint`s.
This relationship looks like it only exists to avoid scattering calls of
`failureManager.handleCheckpointException(exception)` over the coordinator. But
couldn't there just be a helper method in the coordinator that calls the method
on pending checkpoint and the failure manager and is used as replacement for
`PendingCheckpoint#abort`? Or is there any other good reason why this
dependency should or needs to exist?
----------------------------------------------------------------
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
