[FLINK-34516] Move CheckpointingMode to flink-core #24381
Conversation
Zakelly
changed the title
| */ | ||
| @Deprecated | ||
| public CheckpointingMode getCheckpointingMode() { | ||
| return configuration.get(ExecutionCheckpointingOptions.CHECKPOINTING_MODE); |
There was a problem hiding this comment.
IIUC, we should replace all internal usages of original CheckpointMode and docs with new one except for the user-facing interface, right?
I'd suggest to transform to the new CheckpointMode for all original public interfaces, and use the new one for other inner places.
There was a problem hiding this comment.
IIUC, we should replace all internal usages of original CheckpointMode and docs with new one except for the user-facing interface, right?
Yeah, and this will be addressed by another PR.
I'd suggest to transform to the new CheckpointMode for all original public interfaces, and use the new one for other inner places.
Well, I suggest we make this easier since the public API and the option will be removed at the same time, no more complex conversion needed, WDYT?
|
@flinkbot run azure |
Zakelly
force-pushed
the
f34516
branch
4 times, most recently
from
ccc1f1c
to
315e2f4
Compare
|
@masteryhx I have transformed to the new CheckpointMode everywhere, and rebased master. Would you please take a look? |
|
Rebased for conflicts.... |
Zakelly
force-pushed
the
f34516
branch
2 times, most recently
from
ccba6d5
to
b27cdbb
Compare
|
@flinkbot run azure |
1 similar comment
|
@flinkbot run azure |
| * | ||
| * @return The checkpointing mode. | ||
| */ | ||
| public CheckpointingMode getConsistencyMode() { |
There was a problem hiding this comment.
CheckpointingConsistencyMode looks good to me.
Could we also update the config name and the class name since it's also a public API same as config ?
There was a problem hiding this comment.
@masteryhx Personally, I'd be fine with name the enum CheckpointingConsistencyMode, but that's a breaking change and needs more discussion. My current plan is to keep the CheckpointingMode here, and provide API with 'Consistency' in its name just for avoiding conflicts. We will reuse the original API name later in 2.0, WDYT?
There was a problem hiding this comment.
I just have a minor concern that users may lost about CheckpointingMode and ConsistencyMode.
How about renaming this to CheckpointingConsistencyMode and adding some descriptions about the relationship between them ?
There was a problem hiding this comment.
I'd change the API name get/setConsistencyMode to get/setCheckpointingConsistencyMode and add some description showing the relationship between the name CheckpointingConsistencyMode and CheckpointingMode. But I'd suggest keep CheckpointingMode as it is since it is a first class citizen of Flink and widely accepted by users. WDYT?
|
merged 8fac804...7794591 into master |
What is the purpose of the change
In FLIP-406, we want to merge all options in
ExecutionCheckpointingOptionstoCheckpointingOptions, which depends on theCheckpointingModebeing moved toflink-coreas well. This PR introduce a newCheckpointingModeinflink-core, and deprecate the old one as well as corresponding user-facing APIs.Brief change log
CheckpointingModeinflink-core, providing new APIsget/setConsistencyMode.CheckpointingModeinflink-streaming-javaas well as the APIs.Verifying this change
This change is already covered by new introduced UTs.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation