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/08 16:27:15 UTC
[GitHub] [flink] rkhachatryan opened a new pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
rkhachatryan opened a new pull request #13091:
URL: https://github.com/apache/flink/pull/13091
## What is the purpose of the change
`CheckpointRequestDecider` uses lastCheckpointCompletionTime and pendingRequests.size to make a decision.
While latter is currently checked under a lock, the former is not.
So it can see "no pending checkpoints" (fresh value of `pendingRequests.size`) and "last checkpoint completed long time ago" (stale value of `lastCheckpointCompletionTime`).
Therefore, it decides to execute request, effictevely ignoring minPauseBetweenCheckpoint setting.
This increases checkpoint frequency, and in some cases decreases throughput (see ML thread).
This PR replaces function argument for `lastCheckpointCompletionTime` with a supplier, which is called under lock (similar to `pendingRequests.size()`).
## Verifying this change
Added unit test: `CheckpointCoordinatorTest.testMinCheckpointPause`.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: yes, Checkpointing
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
----------------------------------------------------------------
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
[GitHub] [flink] rkhachatryan commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [flink] pnowojski commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by 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
[GitHub] [flink] flinkbot commented on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670947770
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 6339ff6f9b1acdb6f7cd163c9f866f77fd65c708 (Sat Aug 08 16:29:25 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
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
[GitHub] [flink] flinkbot edited a comment on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670949094
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309",
"triggerID" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"triggerType" : "PUSH"
}, {
"hash" : "3373a315374003a67dc8c2a32e3bdb7aadc80d8d",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5460",
"triggerID" : "3373a315374003a67dc8c2a32e3bdb7aadc80d8d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3373a315374003a67dc8c2a32e3bdb7aadc80d8d Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5460)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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
[GitHub] [flink] rkhachatryan commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on a change in pull request #13091:
URL: https://github.com/apache/flink/pull/13091#discussion_r467631120
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
##########
@@ -318,6 +318,7 @@ public CheckpointCoordinator(
this.clock,
this.minPauseBetweenCheckpoints,
this.pendingCheckpoints::size,
+ () -> this.lastCheckpointCompletionRelativeTime,
Review comment:
Thanks for reviewing @klion26.
`volatile` seems unnecessary because (both) values are written and read under the same lock.
----------------------------------------------------------------
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
[GitHub] [flink] flinkbot commented on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670949094
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6339ff6f9b1acdb6f7cd163c9f866f77fd65c708 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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
[GitHub] [flink] flinkbot edited a comment on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670949094
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309",
"triggerID" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"triggerType" : "PUSH"
}, {
"hash" : "3373a315374003a67dc8c2a32e3bdb7aadc80d8d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "3373a315374003a67dc8c2a32e3bdb7aadc80d8d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6339ff6f9b1acdb6f7cd163c9f866f77fd65c708 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309)
* 3373a315374003a67dc8c2a32e3bdb7aadc80d8d UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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
[GitHub] [flink] flinkbot edited a comment on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670949094
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309",
"triggerID" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"triggerType" : "PUSH"
}, {
"hash" : "3373a315374003a67dc8c2a32e3bdb7aadc80d8d",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5460",
"triggerID" : "3373a315374003a67dc8c2a32e3bdb7aadc80d8d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6339ff6f9b1acdb6f7cd163c9f866f77fd65c708 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309)
* 3373a315374003a67dc8c2a32e3bdb7aadc80d8d Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5460)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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
[GitHub] [flink] rkhachatryan commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on a change in pull request #13091:
URL: https://github.com/apache/flink/pull/13091#discussion_r469268685
##########
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:
As clarified offline, the proposal was not "remove existing suppliers" but "not to add a new one"; which is indeed a smaller changeset.
I'm updating the PR.
----------------------------------------------------------------
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
[GitHub] [flink] klion26 commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
klion26 commented on a change in pull request #13091:
URL: https://github.com/apache/flink/pull/13091#discussion_r467544272
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
##########
@@ -318,6 +318,7 @@ public CheckpointCoordinator(
this.clock,
this.minPauseBetweenCheckpoints,
this.pendingCheckpoints::size,
+ () -> this.lastCheckpointCompletionRelativeTime,
Review comment:
@rkhachatryan thanks for the fix.
After this change, the behavior is equal to 1.10 and would respect the min-pause setting now.
I'm wandering do we need to make the `lastCheckpointCompletionRelativeTime` and `pendingCheckpoints.size` volatile.
----------------------------------------------------------------
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
[GitHub] [flink] flinkbot edited a comment on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670949094
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309",
"triggerID" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6339ff6f9b1acdb6f7cd163c9f866f77fd65c708 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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
[GitHub] [flink] flinkbot edited a comment on pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13091:
URL: https://github.com/apache/flink/pull/13091#issuecomment-670949094
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309",
"triggerID" : "6339ff6f9b1acdb6f7cd163c9f866f77fd65c708",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6339ff6f9b1acdb6f7cd163c9f866f77fd65c708 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=5309)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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
[GitHub] [flink] rkhachatryan commented on a change in pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on a change in pull request #13091:
URL: https://github.com/apache/flink/pull/13091#discussion_r469268685
##########
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:
As clarified offline, the proposal was not "remove existing suppliers" but "not to add a new one"; which is indeed a smaller changeset.
I've updated the PR, please take a look @pnowojski .
----------------------------------------------------------------
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
[GitHub] [flink] pnowojski merged pull request #13091: [FLINK-18856][checkpointing] Don't ignore minPauseBetweenCheckpoints
Posted by GitBox <gi...@apache.org>.
pnowojski merged pull request #13091:
URL: https://github.com/apache/flink/pull/13091
----------------------------------------------------------------
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