xintongsong commented on code in PR #24163:
URL: https://github.com/apache/flink/pull/24163#discussion_r1462693554
##########
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/kubeclient/Fabric8FlinkKubeClient.java:
##########
@@ -102,6 +109,14 @@ public Fabric8FlinkKubeClient(
this.maxRetryAttempts =
flinkConfig.getInteger(
KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_MAX_RETRIES);
+ this.initialRetryInterval =
+ flinkConfig.getOptional(
+
KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_INITIAL_RETRY_DEALY)
+
.orElse(KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_INITIAL_RETRY_DEALY.defaultValue());
+ this.maxRetryInterval =
+ flinkConfig.getOptional(
+
KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_MAX_RETRY_DEALY)
+
.orElse(KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_MAX_RETRY_DEALY.defaultValue());
Review Comment:
There's no need to use `getOptional().orElse()`. A simple `get()` should
automatically apply the default value if not configured.
```suggestion
this.initialRetryInterval =
flinkConfig.get(
KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_INITIAL_RETRY_DEALY);
this.maxRetryInterval =
flinkConfig.get(
KubernetesConfigOptions.KUBERNETES_TRANSACTIONAL_OPERATION_MAX_RETRY_DEALY);
```
##########
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java:
##########
@@ -440,6 +441,28 @@ public class KubernetesConfigOptions {
code("FlinkKubeClient#checkAndUpdateConfigMap"))
.build());
+ public static final ConfigOption<Duration>
KUBERNETES_TRANSACTIONAL_OPERATION_INITIAL_RETRY_DEALY =
+ key("kubernetes.transactional-operation.initial-retry-delay")
+ .durationType()
+ .defaultValue(Duration.ofMillis(5000))
Review Comment:
I think the 5s default initial delay might be too large. I'd suggest maybe
50ms.
##########
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java:
##########
@@ -440,6 +441,28 @@ public class KubernetesConfigOptions {
code("FlinkKubeClient#checkAndUpdateConfigMap"))
.build());
+ public static final ConfigOption<Duration>
KUBERNETES_TRANSACTIONAL_OPERATION_INITIAL_RETRY_DEALY =
+ key("kubernetes.transactional-operation.initial-retry-delay")
+ .durationType()
+ .defaultValue(Duration.ofMillis(5000))
Review Comment:
That also means it would take 12 retries to reach the default 1m max delay.
Maybe we can also increase the default value for max-retires to maybe 15 or 20
times?
--
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]
