StefanRRichter commented on a change in pull request #7571: [FLINK-10724]
Refactor failure handling in check point coordinator
URL: https://github.com/apache/flink/pull/7571#discussion_r276163397
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java
##########
@@ -101,7 +100,7 @@
private final CheckpointStorageLocation targetLocation;
/** The promise to fulfill once the checkpoint has been completed. */
- private final CompletableFuture<CompletedCheckpoint>
onCompletionPromise;
+ private final CompletableFuture<CheckpointExecutionResult>
onCompletionPromise;
Review comment:
Basically that sounds very close to what I was suggesting, except that I
think we don't need a dedicated `CheckpointResult`. I would imagine this as
follows:
- Success case can simply use CompletedCheckpoint or even just the
checkpoint id, except if there is a good reason why we need to report more
information that we need a bundle class like this.
- Exception cases can report a new type of exception (e.g.
`CheckpointException`) with an enum field that carries the reason (e.g.
`CheckpointFailureReason`).
- The manager has the corresponding methods to invoke for those cases.
Now I think there are only two questions left for me:
- do we even need `CheckpointResult` or is that unnecessary boilerplate.
- to we only want one type `CheckpointException` or is there a benefit in
having two distict exceptions for the trigger case and the execution case. Do
you see any advantages to gain from two over one or not?
----------------------------------------------------------------
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
