[GitHub] [flink] rkhachatryan commented on a diff in pull request #19864: [FLINK-27162][runtime] Trigger non-periodic checkpoint in 'timer' thread

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

rkhachatryan commented on code in PR #19864:
URL: https://github.com/apache/flink/pull/19864#discussion_r911913272


##########
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java:
##########
@@ -493,6 +498,15 @@ private CompletableFuture<CompletedCheckpoint> triggerSavepointInternal(
         return resultFuture;
     }
 
+    /**
+     * @param isPeriodic Flag indicating whether this triggered checkpoint is periodic.
+     * @return a future to the completed checkpoint.
+     */
+    public CompletableFuture<CompletedCheckpoint> triggerCheckpointFromCheckpointThread(
+            boolean isPeriodic) {

Review Comment:
   I think this method should not accept `boolean isPeriodic`:
   If periodic triggering is enabled via `startCheckpointScheduler()`, I couldn't imagine a use case for that - and there is no callers using `true`.
   If it's disabled with `stopCheckpointScheduler()` then it doesn't make sense to call this method at all.



##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/StateWithExecutionGraph.java:
##########
@@ -286,7 +286,7 @@ CompletableFuture<String> triggerCheckpoint() {
         logger.info("Triggering a checkpoint for job {}.", jobID);
 
         return checkpointCoordinator
-                .triggerCheckpoint(false)
+                .triggerCheckpointFromCheckpointThread(false)

Review Comment:
   The old method can now be made private. Then I'd named it `triggerCheckpointInternal` and name the new one (which uses `timer.execute`) `triggerCheckpoint`. 
   The caller of the public method shouldn't chose which thread to use IMO.
   WDYT?



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org