rkhachatryan commented on a change in pull request #13091:
URL: https://github.com/apache/flink/pull/13091#discussion_r467853626
##########
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:
I agree that the current design has these two issues:
1. Unnecessary complex
2. Unclear synchronization semantics
To deal with both of them I
[proposed](https://docs.google.com/document/d/1p0m4FAmpWxShFaicgHXKqKCHYjnsDnKzvyU7BuA9CT8/edit?usp=sharing)
to use actor-based approach.
OTH, the proposed change of "making CheckpointRequestDecider a non thread
safe class":
1. replaces Suppliers with arguments, but doesn't remove the complexity
1. moves synchronization to another class, but doesn't remove it (we'll need
to synchronize more sections in `CheckpointCoordinator`)
I like these changes, but not in the context of the upcoming refactoring
(even if not actor-based - I tried to follow the original sync logic when
extracting `CheckpointRequestDecider`, so changing it now can make the
refactoring more difficult).
Besides that, the fix should be backported to 1.11, so I'd prefer a smaller
changeset.
----------------------------------------------------------------
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]
