[GitHub] [flink] pnowojski commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints

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

pnowojski commented on a change in pull request #13091:
URL: https://github.com/apache/flink/pull/13091#discussion_r467832735



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointRequestDecider.java
##########
@@ -55,20 +55,23 @@
 	@GuardedBy("lock")
 	private final NavigableSet<CheckpointTriggerRequest> queuedRequests = new TreeSet<>(checkpointTriggerRequestsComparator());
 	private final int maxQueuedRequests;
+	private final Supplier<Long> completionTimeSupplier;
 
 	CheckpointRequestDecider(
 			int maxConcurrentCheckpointAttempts,
 			Consumer<Long> rescheduleTrigger,
 			Clock clock,
 			long minPauseBetweenCheckpoints,
 			Supplier<Integer> pendingCheckpointsSizeSupplier,
+			Supplier<Long> completionTimeSupplier,

Review comment:
       Those suppliers/consumers are making this class harder to follow and I think they are showing some deeper design problem - too tight coupling between `CheckpointCoordinator`/`CheckpointRequestDecider`.
   
   In particular, it looks like adding `Supplier<Long> completionTimeSupplier` reveals quite weird concurrency contract between those two. Both are using the same lock object, and they are cross accessing the `CheckpointCoordinator#lastCheckpointCompletionRelativeTime` field from within the lock `CheckpointRequestDecider#lock` for read access and `CheckpointCoordinator#lock` for the write access.
   
   What about making `CheckpointRequestDecider` a non thread safe class, remove the `CheckpointRequestDecider#lock` and just relay on the caller (`CheckpointCoordinator`) to either access `CheckpointRequestDecider` from a single thread or provide thread safety. For now `CheckpointCoordinator` would be acquiring `CheckpointCoordinator#lock` before calling `CheckpointRequestDecider`, while after completing threading model refactor, the `CheckpointCoordinator#lock` would be simply removed, without a need to modify `CheckpointRequestDecider`?




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