Airblader commented on a change in pull request #17893:
URL: https://github.com/apache/flink/pull/17893#discussion_r756334533
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/ExecutionCheckpointingOptions.java
##########
@@ -88,16 +88,18 @@
+ "we do not tolerance any checkpoint
failure.");
public static final
ConfigOption<CheckpointConfig.ExternalizedCheckpointCleanup>
- EXTERNALIZED_CHECKPOINT =
+ EXTERNALIZED_CHECKPOINT_CLEANUP =
Review comment:
This is a breaking change. If it isn't necessary, we should avoid it;
otherwise we can keep a deprecated alias for now.
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/CheckpointConfig.java
##########
@@ -711,7 +713,7 @@ public long getCheckpointIdOfIgnoredInFlightData() {
* <p>Note that checkpoint state is always kept if the job terminates
with state {@link
* JobStatus#FAILED}.
*/
- DELETE_ON_CANCELLATION(true),
+ DELETE_ON_CANCELLATION,
Review comment:
Since we're at it, we could have the enum implement `DescribedEnum` and
clean up the description of the config option to not mention the enum values in
the text (this will then be generated from the enum).
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/ExecutionCheckpointingOptions.java
##########
@@ -88,16 +88,18 @@
+ "we do not tolerance any checkpoint
failure.");
public static final
ConfigOption<CheckpointConfig.ExternalizedCheckpointCleanup>
- EXTERNALIZED_CHECKPOINT =
+ EXTERNALIZED_CHECKPOINT_CLEANUP =
ConfigOptions.key("execution.checkpointing.externalized-checkpoint-retention")
.enumType(CheckpointConfig.ExternalizedCheckpointCleanup.class)
- .noDefaultValue()
+ .defaultValue(
+
CheckpointConfig.ExternalizedCheckpointCleanup
+ .NO_EXTERNALIZED_CHECKPOINTS)
.withDescription(
Description.builder()
.text(
"Externalized checkpoints
write their meta data out to persistent storage and are not "
+ "automatically
cleaned up when the owning job fails or is suspended (terminating with job "
- + "status %s or
%s. In this case, you have to manually clean up the checkpoint state, both the "
+ + "status %s or
%s). In this case, you have to manually clean up the checkpoint state, both the
"
Review comment:
As mentioned above, we should not describe the options here but let the
enum do that.
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/environment/CheckpointConfig.java
##########
@@ -446,7 +447,7 @@ public void setTolerableCheckpointFailureNumber(int
tolerableCheckpointFailureNu
* @param cleanupMode Externalized checkpoint cleanup behaviour.
*/
@PublicEvolving
- public void enableExternalizedCheckpoints(ExternalizedCheckpointCleanup
cleanupMode) {
+ public void setExternalizedCheckpointCleanup(ExternalizedCheckpointCleanup
cleanupMode) {
Review comment:
Renaming this is a breaking change. We should probably keep the old
method, too, and deprecate it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
