You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/11 15:51:48 UTC
[GitHub] [flink-kubernetes-operator] bgeng777 opened a new pull request, #204: [Flink 27329] Add default value of replica of JM pod and not declare it in example yamls
bgeng777 opened a new pull request, #204:
URL: https://github.com/apache/flink-kubernetes-operator/pull/204
1. Revisit default values in org.apache.flink.kubernetes.operator.crd.spec (details is [here])(https://issues.apache.org/jira/browse/FLINK-27329?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel)
2. Set default value of `crd.spec.Resource#cpu` to `1.0`
3. Set default value of `crd.spec.FlinkDeploymentSpec#serviceAccount` to `flink`
4. Set default value of `crd.spec.JobManagerSpec#replicas` to 1 and remove its usage in examples.
--
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-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 diff in pull request #204: [Flink 27329] Add default value of replica of JM pod and not declare it in example yamls
Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #204:
URL: https://github.com/apache/flink-kubernetes-operator/pull/204#discussion_r870507063
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/FlinkDeploymentSpec.java:
##########
@@ -45,7 +45,7 @@ public class FlinkDeploymentSpec extends AbstractFlinkSpec {
private String imagePullPolicy;
/** Kubernetes service used by the Flink deployment. */
- private String serviceAccount;
+ private String serviceAccount = "flink";
Review Comment:
I suggest to leave it on `null` and throw a validation error if not set. Accidentally using the wrong service account could be bad
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora merged pull request #204: [Flink 27329] Add default value of replica of JM pod and not declare it in example yamls
Posted by GitBox <gi...@apache.org>.
gyfora merged PR #204:
URL: https://github.com/apache/flink-kubernetes-operator/pull/204
--
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-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 diff in pull request #204: [Flink 27329] Add default value of replica of JM pod and not declare it in example yamls
Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #204:
URL: https://github.com/apache/flink-kubernetes-operator/pull/204#discussion_r870507063
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/FlinkDeploymentSpec.java:
##########
@@ -45,7 +45,7 @@ public class FlinkDeploymentSpec extends AbstractFlinkSpec {
private String imagePullPolicy;
/** Kubernetes service used by the Flink deployment. */
- private String serviceAccount;
+ private String serviceAccount = "flink";
Review Comment:
I suggest to leave it on `null` and throw a validation error if not set.
--
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-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 diff in pull request #204: [Flink 27329] Add default value of replica of JM pod and not declare it in example yamls
Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #204:
URL: https://github.com/apache/flink-kubernetes-operator/pull/204#discussion_r870952754
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java:
##########
@@ -385,4 +386,12 @@ private Optional<String> validateSpecChange(FlinkSessionJob sessionJob) {
return Optional.empty();
}
+
+ private Optional<String> validateServiceAccount(String serviceAccount) {
+ if (serviceAccount == null) {
+ return Optional.of(
+ "serviceAccount must be defined. If you use helm, its value should be the same with the name of jobServiceAccount.");
Review Comment:
I think it would be better to say `spec.serviceAccount must be defined. ...` so the user knows where exactly it's missing.
--
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-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 diff in pull request #204: [Flink 27329] Add default value of replica of JM pod and not declare it in example yamls
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #204:
URL: https://github.com/apache/flink-kubernetes-operator/pull/204#discussion_r870931220
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/FlinkDeploymentSpec.java:
##########
@@ -45,7 +45,7 @@ public class FlinkDeploymentSpec extends AbstractFlinkSpec {
private String imagePullPolicy;
/** Kubernetes service used by the Flink deployment. */
- private String serviceAccount;
+ private String serviceAccount = "flink";
Review Comment:
thanks for the suggestion. It makes sense to me to add validation check so that serviceAccount will be a required field.
For users who use helm and set a different serviceAccount, they are supposed to be aware of using correct service account.
For users who use helm and use the default serviceAccount, we may add some comments in examples or detailed validation error info to alert users to set the correct serviceAccount(i.e. `flink`).
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org