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-21505] Enforce common/unified savepoint format at operator level #14982
[FLINK-21505] Enforce common/unified savepoint format at operator level #14982
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 69e3b6f (Mon Feb 22 10:29:32 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:
|
Generally speaking I like the idea. I think it is a nice way to ensure all state backends produce a common unified format. I will go over the PR and do a proper review. |
.../src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackendBuilder.java
Outdated
Show resolved
Hide resolved
...-java/src/main/java/org/apache/flink/streaming/api/operators/StreamOperatorStateHandler.java
Outdated
Show resolved
Hide resolved
"Asynchronous full Savepoint", | ||
savepointSnapshotStrategy, | ||
closeableRegistry, | ||
ASYNCHRONOUS); |
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.
Do we want to hardcode ASYNCHRONOUS
here?
flink-tests/src/test/java/org/apache/flink/test/state/HeapSavepointStateBackendSwitchTest.java
Show resolved
Hide resolved
da7d208
to
99de033
Compare
…hotStrategy This will allow us to create snapshot resources for a savepoint without using a snapshot strategy.
56576b4
to
1738289
Compare
1738289
to
912c9b4
Compare
… level Before, we were relying on the fact that all keyed backends would use the same strategy for savepoints. Now, we're forcing them at the API level to provide a SavepointResources that we can then use to create a unified savepoint using SavepointSnapshotStrategy.
912c9b4
to
58fdc4f
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.
Good job! I really like the flow now!
@flinkbot run azure |
1 similar comment
@flinkbot run azure |
I think the problem with azure is that it is not rebased on the fixed master. I have rebased the PR on current master and I am running it on my private azure: https://dev.azure.com/wysakowiczdawid/Flink/_build/results?buildId=707&view=results |
Before, we were relying on the fact that all keyed backends would use the same strategy for savepoints.
Now, we're forcing them at the API level to provide a
SavepointResources
that we can then use to create a unified savepoint usingSavepointSnapshotStrategy
.Verifying this change
This is a refactoring and is covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation