-
Notifications
You must be signed in to change notification settings - Fork 13k
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-26134][docs] Added table with Checkpoint/Savepoint guarantees … #18765
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 8b902f3 (Mon Feb 14 16:49:37 UTC 2022) 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:
|
@infoverload, I need help with the design of this documentation. The idea is to have approximately the same table as here - https://cwiki.apache.org/confluence/display/FLINK/FLIP-203%3A+Incremental+savepoints#FLIP203:Incrementalsavepoints-Proposal.
|
@dawidwys, I have added the description to each table line under the table. I don't think that my clarification is perfect but it is all that I have. So if you have ideas on how to make it more clear, please, share them with me. Also, if you know doc pages to which I can link, give me know. |
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.
I have left some comments as suggestions on how we could improve the referencing/integration into the rest of the docs.
The description of "Arbitrary job upgrad" and "Job upgrade w/o ..." weren't clear to me.
I think expanding the definitions under the table is fine. |
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.
Thanks for creating this PR, I feel doubted about the description of relocatable native savepoint of current implementation.
I will also verify it later.
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.
@dawidwys, what is the page with CLI documentation that you told about?
This is the cli page: https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/cli/#creating-a-savepoint and this is a pending PR I mentioned: #18900 |
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.
@akalash Is it okay if I make some changes (commits) on top. I would try to correct some minor language issues which is cumbersome to do via review comments. Is that okay for you?
@smattheis, since the branch is from in my personal repository, it will be not convenient for you to add new commits on top. I think we usually use something like |
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.
I have made some suggestions which are just suggestions for better merging/integration of paragraphs. If I made it worse, please feel free to ignore.
@smattheis , I have not applied all of your suggestions but I have applied some of the ideas. So please, take a look again when you have time |
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.
| State Processor API(writing) | ✓ | x | x | x | | ||
| State Processor API(reading) | ✓ | ! | ! | x | | ||
| Self-contained and relocatable | ✓ | ✓ | x | x | | ||
| Schema evolution | ✓ | ! | ! | ! | |
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.
Are the values for Schema evolution
correct?
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.
I think yes, according to FLIP(https://cwiki.apache.org/confluence/display/FLINK/FLIP-203%3A+Incremental+savepoints#FLIP203:Incrementalsavepoints-Proposal) Canonical Savepoint is supported while all others are unofficially supported.
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.
Haven't we said we want to either update it, or remove it from the current version of the table? @pnowojski
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.
I don't remember. Have we?
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.
It's going to be very helpful to have these details spelled out in the documentation.
I've provided some suggested fixups for the English -- some of which might introduce undesirable changes to the meaning, so please check them carefully.
And one overall note: I find it distracting that Checkpoint is often capitalized (even when it's not the first word in a sentence). I'd prefer use we use lowercase for checkpoints and savepoints, which I believe matches the style of the rest of the documentation.
@alpinegizmo , thanks a lot for your suggestions. I have made changes. Please, take a look once again when you have time. |
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.
Approving this now, but I noticed a few minor things that can be improved and offered suggestions for them.
…into documentation
What is the purpose of the change
Moved table with snapshot guarantees clarification from https://cwiki.apache.org/confluence/display/FLINK/FLIP-203%3A+Incremental+savepoints#FLIP203:Incrementalsavepoints-Proposal
Brief change log
Verifying this change
it is documentation
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation