XComp commented on a change in pull request #14847:
URL: https://github.com/apache/flink/pull/14847#discussion_r581982820
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/SchedulerBase.java
##########
@@ -835,8 +840,21 @@ public void updateAccumulators(final AccumulatorSnapshot
accumulatorSnapshot) {
mainThreadExecutor);
}
- private void startCheckpointScheduler(final CheckpointCoordinator
checkpointCoordinator) {
+ @Override
+ public void stopCheckpointScheduler() {
+ Preconditions.checkState(
+ getCheckpointCoordinator() != null,
+ "Checkpointing cannot be stopped since it's not enabled.");
+ getCheckpointCoordinator().stopCheckpointScheduler();
+ }
+
+ @Override
+ public void startCheckpointScheduler() {
mainThreadExecutor.assertRunningInMainThread();
+ final CheckpointCoordinator checkpointCoordinator =
getCheckpointCoordinator();
+ Preconditions.checkState(
+ checkpointCoordinator != null,
+ "Checkpointing cannot be started since it's not enabled.");
Review comment:
I agree: That's a valid scenario. I'm just wondering now whether we
should simply make the call optional in that case. We could just add another
condition to the if statement. I don't like that because it makes the interface
less specific. On the other hand one could argue that starting the checkpoint
scheduling was always optional due to the condition that checks whether
periodic checkpointing is enabled.
Another alternative would be to extend the interface by a method to check
the state of the checkpointing (like `CheckpointCoordinator.shutdown` and only
re-enable it if it's not shutdown. @rmetzger any thoughts on this.
----------------------------------------------------------------
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]
