gyfora commented on code in PR #634:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/634#discussion_r1269015750
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java:
##########
@@ -525,4 +525,12 @@ public static String operatorConfigKey(String key) {
.enumType(DeletionPropagation.class)
.defaultValue(DeletionPropagation.FOREGROUND)
.withDescription("JM/TM Deployment deletion propagation.");
+
+ @Documentation.Section(SECTION_DYNAMIC)
+ public static final ConfigOption<Boolean> SAVEPOINT_ON_DELETION =
+ operatorConfig("savepoint.on.deletion")
Review Comment:
We did not manage to have very consistent config names so far but maybe
`job.savepoint-on-deletion` would be a bit better :)
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java:
##########
@@ -292,6 +294,14 @@ protected void resubmitJob(FlinkResourceContext<CR> ctx,
boolean requireHaMetada
restoreJob(ctx, specToRecover, ctx.getObserveConfig(),
requireHaMetadata);
}
+ protected void cancelJob(FlinkResourceContext<CR> ctx) throws Exception {
Review Comment:
Do we need to introduce a new method? I would prefer to keep the existing
ones because the third method with the same name is getting a bit confusing
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java:
##########
@@ -292,6 +294,14 @@ protected void resubmitJob(FlinkResourceContext<CR> ctx,
boolean requireHaMetada
restoreJob(ctx, specToRecover, ctx.getObserveConfig(),
requireHaMetadata);
}
+ protected void cancelJob(FlinkResourceContext<CR> ctx) throws Exception {
+ UpgradeMode upgradeMode =
+
configManager.getOperatorConfiguration().isSavepointOnDeletion()
Review Comment:
I have recently reworked the operatorConfig handling to allow for more
flexible default value handling. The ctx now provides a method to get the
operator config directly so you don't need the configmanager.
`ctx.getOperatorConfig()`
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
