You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/08/03 09:41:20 UTC
[GitHub] [flink] rkhachatryan commented on a change in pull request #13045: [FLINK-18748][checkpointing] trigger unperiodic checkpoint immediately
rkhachatryan commented on a change in pull request #13045:
URL: https://github.com/apache/flink/pull/13045#discussion_r464287882
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointRequestDecider.java
##########
@@ -136,6 +136,11 @@
.map(unused -> queuedRequests.pollFirst());
}
+ CheckpointTriggerRequest first = queuedRequests.first();
+ if (first.isForce() || !first.isPeriodic) {
Review comment:
Can we make the connection between this check and `onTooEarly` more explicit?
For example, by inverting this condition and putting `onTooEarly` inside.
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointRequestDeciderTest.java
##########
@@ -119,13 +119,13 @@ public void testQueueSizeLimitPriority() {
public void testSavepointTiming() {
testTiming(regularSavepoint(), TriggerExpectation.IMMEDIATELY);
testTiming(periodicSavepoint(), TriggerExpectation.IMMEDIATELY);
- testTiming(nonForcedSavepoint(), TriggerExpectation.AFTER_PAUSE);
+ testTiming(nonForcedSavepoint(), TriggerExpectation.IMMEDIATELY);
Review comment:
I think maxPendingCheckpoints limit (what @klion26 proposed) and order are different things to check.
For the 1st one, I think we can reuse `testEnqueueOnTooManyPending` by calling `nonForcedSavepoint` instead of `regularCheckpoint`.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointRequestDecider.java
##########
@@ -146,15 +151,11 @@
private Optional<CheckpointTriggerRequest> onTooEarly(long nextTriggerDelayMillis) {
CheckpointTriggerRequest first = queuedRequests.first();
- if (first.isForce()) {
- return Optional.of(queuedRequests.pollFirst());
- } else if (first.isPeriodic) {
+ if (first.isPeriodic) {
Review comment:
Can we inline this function now?
Otherwise, we need to duplicate the check and return logic (or use preconditions).
Currently, I think `if` is missing `&& !isForce` check,
and in the end, it should return `pollFirst`.
----------------------------------------------------------------
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