-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-23041][streaming] Added new well defined checkpoint configuration aligned-checkpoint-timeout instead of alignment-timeout #16227
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 4fa243a (Thu Sep 23 17:53:02 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. 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 Bot commandsThe @flinkbot bot supports the following commands:
|
757fe73
to
9a20519
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Had one comment to the code. Could you also update the UC documentation page? https://ci.apache.org/projects/flink/flink-docs-master/docs/ops/state/unaligned_checkpoints/ I think adding a sentence or two about what does the timeout mean would be a nice touch.
...treaming-java/src/main/java/org/apache/flink/streaming/api/environment/CheckpointConfig.java
Outdated
Show resolved
Hide resolved
...est/java/org/apache/flink/streaming/runtime/io/checkpointing/AlternatingCheckpointsTest.java
Outdated
Show resolved
Hide resolved
@@ -207,6 +208,7 @@ public int hashCode() { | |||
checkpointRetentionPolicy, | |||
isExactlyOnce, | |||
isUnalignedCheckpointsEnabled, | |||
alignedCheckpointTimeout, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
@@ -101,10 +101,10 @@ | |||
"type" : "object", | |||
"id" : "urn:jsonschema:org:apache:flink:runtime:rest:messages:DashboardConfiguration:Features", | |||
"properties" : { | |||
"web-submit" : { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity. Do you know where does the reordering comes from? Is the file autogenerated? If you don't know don't bother looking for an answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is autogenerated but it looks like it is generated in alphabet order so it should be correct now. I can only guess but perhaps, initially, the author made these changes manually
… to UC is based on the global checkpoint start time rather than the local first barrier received time
…n aligned-checkpoint-timeout instead of alignment-timeout
0f55f18
to
8613d50
Compare
@dawidwys, I added the documentation - https://github.com/apache/flink/pull/16227/files#diff-330cebde422d7911fd9cb5f32e58770389b13862cb372e6fdb276ba44bb3c4ac, I don't really sure what should I do with the old alignment timeout but as you can see I just deprecated it. I hope it is ok |
Let's just drop the old value. It wasn't properly advertised and I think it doesn't make much sense to document it on that general page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is the purpose of the change
This PR adds new configuration aligned-checkpoint-timeout instead of alignment-timeout
Brief change log
-alignment-timeout was deprecated
-aligned-checkpoint-timeout was added. It represents time since the global start of the checkpoint after which AC switches to UC
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation