You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@flink.apache.org by GitBox <gi...@apache.org> on 2022/03/01 06:45:37 UTC
[GitHub] [flink-kubernetes-operator] bgeng777 opened a new pull request #29: [FLINK-26405] Add validation check of num of JM replica
bgeng777 opened a new pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29
- Add JM replicas check in the validator to make sure when HA is enabled, the num of JM should be 1.
- Add the relevant test.
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] asfgit closed pull request #29: [FLINK-26405] Add validation check of num of JM replica
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 commented on a change in pull request #29: [FLINK-26405] Add validation check of num of JM replica
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29#discussion_r816555181
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java
##########
@@ -86,12 +87,26 @@
return Optional.empty();
}
- private Optional<String> validateJmSpec(JobManagerSpec jmSpec) {
+ private Optional<String> validateJmSpec(JobManagerSpec jmSpec, Map<String, String> confMap) {
if (jmSpec == null) {
return Optional.empty();
}
- return validateResources("JobManager", jmSpec.getResource());
+ return firstPresent(
+ validateResources("JobManager", jmSpec.getResource()),
+ validateJmReplicas("JobManager", jmSpec.getReplicas(), confMap));
+ }
+
+ private Optional<String> validateJmReplicas(
+ String component, int replicas, Map<String, String> confMap) {
+ if (!confMap.containsKey(HighAvailabilityOptions.HA_MODE.key()) && replicas != 1) {
Review comment:
We should use `HighAvailabilityMode.isHighAvailabilityModeActivated(conf)` instead here.
The following is the pre-check in Flink.
```
public int getReplicas() {
final int replicas =
flinkConfig.get(KubernetesConfigOptions.KUBERNETES_JOBMANAGER_REPLICAS);
if (replicas < 1) {
throw new IllegalConfigurationException(
String.format(
"'%s' should not be configured less than one.",
KubernetesConfigOptions.KUBERNETES_JOBMANAGER_REPLICAS.key()));
} else if (replicas > 1
&& !HighAvailabilityMode.isHighAvailabilityModeActivated(flinkConfig)) {
throw new IllegalConfigurationException(
"High availability should be enabled when starting standby JobManagers.");
}
return replicas;
}
```
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on a change in pull request #29: [FLINK-26405] Add validation check of num of JM replica
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29#discussion_r816577823
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java
##########
@@ -86,12 +87,26 @@
return Optional.empty();
}
- private Optional<String> validateJmSpec(JobManagerSpec jmSpec) {
+ private Optional<String> validateJmSpec(JobManagerSpec jmSpec, Map<String, String> confMap) {
if (jmSpec == null) {
return Optional.empty();
}
- return validateResources("JobManager", jmSpec.getResource());
+ return firstPresent(
+ validateResources("JobManager", jmSpec.getResource()),
+ validateJmReplicas("JobManager", jmSpec.getReplicas(), confMap));
+ }
+
+ private Optional<String> validateJmReplicas(
+ String component, int replicas, Map<String, String> confMap) {
+ if (!confMap.containsKey(HighAvailabilityOptions.HA_MODE.key()) && replicas != 1) {
Review comment:
you are right @wangyang0918 that would be a bit cleaner, we could push a small fixup for this or include this in some followup work.
Sorry for merging this quickly
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on pull request #29: [FLINK-26405] Add validation check of num of JM replica
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29#issuecomment-1055091760
cc @wangyang0918 @gyfora
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a change in pull request #29: [FLINK-26405] Add validation check of num of JM replica
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on a change in pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29#discussion_r816621065
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java
##########
@@ -86,12 +87,26 @@
return Optional.empty();
}
- private Optional<String> validateJmSpec(JobManagerSpec jmSpec) {
+ private Optional<String> validateJmSpec(JobManagerSpec jmSpec, Map<String, String> confMap) {
if (jmSpec == null) {
return Optional.empty();
}
- return validateResources("JobManager", jmSpec.getResource());
+ return firstPresent(
+ validateResources("JobManager", jmSpec.getResource()),
+ validateJmReplicas("JobManager", jmSpec.getReplicas(), confMap));
+ }
+
+ private Optional<String> validateJmReplicas(
+ String component, int replicas, Map<String, String> confMap) {
+ if (!confMap.containsKey(HighAvailabilityOptions.HA_MODE.key()) && replicas != 1) {
Review comment:
My bad for using wrong judgement method. I have followed Yang's advice to fix it in this new #30.
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java
##########
@@ -86,12 +87,26 @@
return Optional.empty();
}
- private Optional<String> validateJmSpec(JobManagerSpec jmSpec) {
+ private Optional<String> validateJmSpec(JobManagerSpec jmSpec, Map<String, String> confMap) {
if (jmSpec == null) {
return Optional.empty();
}
- return validateResources("JobManager", jmSpec.getResource());
+ return firstPresent(
+ validateResources("JobManager", jmSpec.getResource()),
+ validateJmReplicas("JobManager", jmSpec.getReplicas(), confMap));
+ }
+
+ private Optional<String> validateJmReplicas(
+ String component, int replicas, Map<String, String> confMap) {
+ if (!confMap.containsKey(HighAvailabilityOptions.HA_MODE.key()) && replicas != 1) {
Review comment:
My bad for using wrong judgement method. I have followed Yang's advice to fix it in this PR #30.
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 commented on a change in pull request #29: [FLINK-26405] Add validation check of num of JM replica
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #29:
URL: https://github.com/apache/flink-kubernetes-operator/pull/29#discussion_r816600887
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java
##########
@@ -86,12 +87,26 @@
return Optional.empty();
}
- private Optional<String> validateJmSpec(JobManagerSpec jmSpec) {
+ private Optional<String> validateJmSpec(JobManagerSpec jmSpec, Map<String, String> confMap) {
if (jmSpec == null) {
return Optional.empty();
}
- return validateResources("JobManager", jmSpec.getResource());
+ return firstPresent(
+ validateResources("JobManager", jmSpec.getResource()),
+ validateJmReplicas("JobManager", jmSpec.getReplicas(), confMap));
+ }
+
+ private Optional<String> validateJmReplicas(
+ String component, int replicas, Map<String, String> confMap) {
+ if (!confMap.containsKey(HighAvailabilityOptions.HA_MODE.key()) && replicas != 1) {
Review comment:
Never mind. We could always do this in the follow-up work :)
The main concern is that configuration contains `HighAvailabilityOptions.HA_MODE.key()` does not mean it is high available since it could be configured to `none`.
--
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: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org