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-34456][configuration]Move all checkpoint-related options into CheckpointingOptions #24374
base: master
Are you sure you want to change the base?
Conversation
@flinkbot run azure |
@flinkbot run azure |
@flinkbot run azure |
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.
Hi @spoon-lz , thanks for your contribution! At first glance of your PR, I would suggest keep the old org.apache.flink.streaming.api.CheckpointingMode
and mark as deprecated. However it is required to start a discussion about such deprecation on dev mailing list first. I will start the discussion and work on the deprecation in Flink 1.19 (will soon be released this month).
If |
I suggest both keep the old one and the new introduced one. All the internal usage will use the new one, while only user-facing APIs leverage the old one. We could wait the conclusion of the discussion. |
@spoon-lz Sorry for the late reply, seems we come to an agreement in discussion. To be brief, we agree to deprecate the old class and introduce a new one. Corresponding APIs will be changed accordingly. |
I think this is much easier since it is only annotated with @\PublicEvolving. I think we can do this in a similar way without another discussion. A seperated PR is also good. |
@Zakelly A separate PR seems more reasonable, I will submit a separate PR to split |
@Zakelly There are some references to |
@spoon-lz |
…CheckpointingOptions
c9987ee
to
896608a
Compare
@flinkbot run azure |
@Zakelly New code has been submitted. |
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.
@spoon-lz Thanks for the update! There seems to be a conflict. And I think you should re-generate the document as flink-docs/README.md
says. The documents reference among each other should also be updated if there is any.
ConfigOptions.key("execution.checkpointing.mode") | ||
.enumType(CheckpointingMode.class) | ||
.defaultValue(CheckpointingMode.EXACTLY_ONCE) | ||
.withDescription("The checkpointing mode (exactly-once vs. at-least-once)."); |
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 thought we don't need to move those deprecated ones.
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.
You're right, I will delete these deprecated options
@Zakelly The code has been modified, but I don’t know why. After I force push to the current branch, I can see the modification in my warehouse, but not in this PR. Do I need to submit a new PR? |
Hi, Could you check your branch 'lz-CheckpointingOptions' in your repo ? Seems it doesn't include new commits. |
@spoon-lz I didn't see any new commit on https://github.com/spoon-lz/flink/tree/lz-CheckpointingOptions |
What is the purpose of the change
According to the FLIP-406,Move all checkpoint-related options that are out of
CheckpointingOptions
intoCheckpointingOptions
. Deprecate the original ones.Brief change log
org.apache.flink.streaming.api.environment.ExecutionCheckpointingOptions
intoorg.apache.flink.configuration.CheckpointingOptions
ExecutionCheckpointingOptions
as deprecatedorg.apache.flink.streaming.api.CheckpointingMode
toorg.apache.flink.configuration.CheckpointingMode
ExternalizedCheckpointCleanup
out ofCheckpointConfig
and move it to flink-core moduleVerifying this change
This change is a minor rework and is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation