yanghua 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_r294067081
 
 

 ##########
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ##########
 @@ -1329,10 +1331,13 @@ private void discardCheckpoint(PendingCheckpoint 
pendingCheckpoint, @Nullable Th
 
                LOG.info("Discarding checkpoint {} of job {}.", checkpointId, 
job, cause);
 
-               if (cause == null || cause instanceof 
CheckpointDeclineException) {
-                       
pendingCheckpoint.abort(CheckpointFailureReason.CHECKPOINT_DECLINED, cause);
+               if (cause == null) {
+                       failPendingCheckpoint(pendingCheckpoint, 
CheckpointFailureReason.CHECKPOINT_DECLINED);
+               } else if (cause instanceof CheckpointException) {
+                       CheckpointException exception = (CheckpointException) 
cause;
+                       failPendingCheckpoint(pendingCheckpoint, 
exception.getCheckpointFailureReason(), cause);
                } else {
-                       
pendingCheckpoint.abort(CheckpointFailureReason.JOB_FAILURE, cause);
+                       failPendingCheckpoint(pendingCheckpoint, 
CheckpointFailureReason.JOB_FAILURE, cause);
 
 Review comment:
   Actually, `CheckpointFailureManager` can take more effect in the future but 
not now. This is an intermediate step of the whole three PRs. In this PR, we 
need to keep compatible with `setFailOnCheckpointingErrors`, it's the most 
important thing. Otherwise, it will change many user behaviors. We have 
considered counting more failure reason before, but it will make more changes 
and make this PR more complex. So your thought is right but not for now.
   
   The purpose of this PR is to introduce the `CheckpointFailureManager` and do 
further residual refactor work for the first PR #7571. 

----------------------------------------------------------------
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:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to