pnowojski commented on code in PR #20852:
URL: https://github.com/apache/flink/pull/20852#discussion_r974198871
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobMasterGateway.java:
##########
@@ -218,13 +220,27 @@ CompletableFuture<String> triggerSavepoint(
final SavepointFormatType formatType,
@RpcTimeout final Time timeout);
+ /**
+ * Triggers taking a checkpoint of the executed job.
+ *
+ * @param checkpointProperties to determine how checkpoint should be taken
or null if the
+ * existing checkpoint property should be used
+ * @param timeout for the rpc call
+ * @return Future which is completed with the savepoint path once completed
Review Comment:
nit: java doc should say it returns `{@link CompletedCheckpoint}` not `path`.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobMasterGateway.java:
##########
@@ -218,13 +220,27 @@ CompletableFuture<String> triggerSavepoint(
final SavepointFormatType formatType,
@RpcTimeout final Time timeout);
+ /**
+ * Triggers taking a checkpoint of the executed job.
+ *
+ * @param checkpointProperties to determine how checkpoint should be taken
or null if the
+ * existing checkpoint property should be used
+ * @param timeout for the rpc call
+ * @return Future which is completed with the savepoint path once completed
+ */
+ CompletableFuture<CompletedCheckpoint> triggerCheckpoint(
Review Comment:
Does it have to return `CompletedCheckpoint`? It makes a bit inconsistent
with the other method. Can not we use checkpoint location as the cache key?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointRetentionPolicy.java:
##########
@@ -24,6 +24,9 @@
@Internal
public enum CheckpointRetentionPolicy {
+ /** Full Checkpoints should be retained on cancellation and failure. */
+ FULL_RETAIN_ON_CANCELLATION,
Review Comment:
Whether the checkpoint is full or not doesn't fit with the
`CheckpointRetentionPolicy`. It should be an independent axis.
If the checkpoint is full or incremental it should be parametrised
independently of the retention policy. Assuming that we want to expose
retention to users - and I don't see why we should be doing this. Is there some
particular reason why have you implemented it like this @leletan ? Such
exposure makes this more complicated for us to maintain code in the future and
it also makes it more complicated for the users. To me it sounds like this
retention policy should come from the `CheckpointCoordinatorConfiguration` and
be the same for manually and automatically triggered checkpoints.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java:
##########
@@ -511,6 +511,20 @@ public CompletableFuture<CompletedCheckpoint>
triggerCheckpoint(boolean isPeriod
return triggerCheckpointFromCheckpointThread(checkpointProperties,
null, isPeriodic);
}
+ /**
+ * Triggers one new checkpoint with the given checkpoint properties. If
the given checkpoint
+ * properties is null, then it will fall back to use the
CheckpointCoordinator's
+ * checkpointProperties. The return value is a future. It completes when
the checkpoint
+ * triggered finishes or an error occurred.
+ *
+ * @param props specifies the properties of the checkpoint to trigger.
+ * @return a future to the completed checkpoint.
+ */
+ public CompletableFuture<CompletedCheckpoint>
triggerCheckpoint(CheckpointProperties props) {
Review Comment:
nit: add `@Nullable` annotation in missing places
--
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]
