[GitHub] [flink] carp84 commented on a change in pull request #9128: [FLINK-13256] Ensure periodical checkpoint still valid when region failover abort pending checkpoints

[GitBox <gi...@apache.org>]

carp84 commented on a change in pull request #9128: [FLINK-13256] Ensure periodical checkpoint still valid when region failover abort pending checkpoints
URL: https://github.com/apache/flink/pull/9128#discussion_r304048806
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ##########
 @@ -1270,11 +1266,30 @@ public void stopCheckpointScheduler() {
 		}
 	}
 
+	/**
+	 * Aborts all the pending checkpoints and check whether {@code currentPeriodicTrigger} has been assigned as null.
+	 * If so, we would create a new trigger as {@code currentPeriodicTrigger}.
+	 *
+	 * @param exception The exception.
+	 */
+	public void abortPendingCheckpointsWithTriggerValidation(CheckpointException exception) {
+		synchronized (lock) {
 
 Review comment:
   I could see we're synchronizing on `lock` to prevent starting multiple checkpoint schedulers not only here but also in the `start/stopCheckpointScheduler` methods. IMHO this interferes with the comment on `lock` which indicates it to only safeguard checkpoint updates, or say manipulation of `pendingCheckpoints`. It's kind of dangerous and using a separate lock for scheduler may be more reasonable, but it's not necessary to be improved in this PR.

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


With regards,
Apache Git Services