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/14 11:12:38 UTC
[GitHub] [flink-kubernetes-operator] Aitozi opened a new pull request, #215: [FLINK-26784] Add label for the session job to help list the session …
Aitozi opened a new pull request, #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215
…jobs in the same session cluster
This PR is meant to add label for the session job to help to list session jobs under the same session.
--
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] Aitozi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126912911
@wangyang0918 Regarding why we need this: although we can manage the job with external job management tools, but we also may need to operate in command line. In the case to operate on all the jobs of a session cluster, eg: delete all the session jobs before shutdown the cluster, it will be useful I think.
I think the mutating admission webhook is a better way. But one shortcoming is it's optional. If disabled, it will not work
--
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] Aitozi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873102772
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
Review Comment:
Moved to a dedicated method.
--
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] Aitozi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873102697
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
The reconciler do not return `UpdateControl` now, If we want to use the update control for it, we should adjust the interface first to return the update control :/ . We have just decide not to do [this](https://issues.apache.org/jira/browse/FLINK-26915) now. I'm not sure whether we need to break it for this PR.
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r872986702
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
Review Comment:
I suggest to extract this into a utility method (we could use computeIfAbsent)
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
Can't we use an update control for this?
--
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] wangyang0918 commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126894901
TBH, I do not like to do the CR `.metadata` or `.spec` updates in the reconciler. Especially, this is only for easier filter, not for operator internal use case. In the production environment, I believe the users could do this manually or covered by external job management tools.
I also have another feeling this is the responsibility of [mutating admission webhook](https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#mutatingadmissionwebhook).
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r872986702
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
Review Comment:
I suggest to extract adding labels into a utility method to keep the reconcile method concise (we could use computeIfAbsent in the helper)
--
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 pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126978207
I think mutating webhooks provide a good alternative solution but I would prefer to revisit this issue after the release.
--
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] Aitozi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873110013
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
Yes, agree with it. I'm ok to rework the interface to work out it. Also cc @gyfora @wangyang0918
--
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] Aitozi closed pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi closed pull request #215: [FLINK-27163] Add label for the session job to help list the session …
URL: https://github.com/apache/flink-kubernetes-operator/pull/215
--
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] morhidi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126724095
LGTM overall. nit: Shouldn't we use a shorter label, e.g:
```
k get sessionjob -l target=basic-session-cluster
```
--
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 pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126978028
I am also leaning toward -1 on the approach in this PR as so far we have explicitly avoided making changes to the metadata/spec for other features.
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873111999
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
Let's see what @gyfora and @wangyang0918 think/suggest.
--
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] Aitozi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126848340
> k get sessionjob --field-selector spec.deploymentName=basic-session-cluster
I tried this way before, but the custom resource seems do not support field selector
```
kubectl get sessionjob --field-selector spec.deploymentName=basic-session-cluster
Error from server (BadRequest): Unable to find "flink.apache.org/v1beta1, Resource=flinksessionjobs" that match label selector "", field selector "spec.deploymentName=basic-session-cluster": field label not supported: spec.deploymentName
kubectl version --short
Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
Client Version: v1.24.0
Kustomize Version: v4.5.4
Server Version: v1.23.3
```
After search the related issues, I think the custom resource can only support field selector by `metadata.namespace` and `metadata.name`
https://github.com/kubernetes/kubernetes/pull/53345
--
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] Aitozi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873111327
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
As I know we have manually patch the status after https://github.com/apache/flink-kubernetes-operator/pull/199 . So this change do not bring the resourceVersion conflicts, I think. Do you mean keep it as now ?
--
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] morhidi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126862712
+1
For the record we need this functionality due: [Enable arbitrary CRD field selectors](https://github.com/kubernetes/kubernetes/issues/53459)
--
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] morhidi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126726044
On second thought we could use something like this for filtering, aren't we?
```
k get --field-selector spec.deploymentName=basic-session-cluster
```
--
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] morhidi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126859076
> > k get sessionjob --field-selector spec.deploymentName=basic-session-cluster
>
> I tried this way before, but the custom resource seems do not support field selector
>
> ```
> kubectl get sessionjob --field-selector spec.deploymentName=basic-session-cluster
> Error from server (BadRequest): Unable to find "flink.apache.org/v1beta1, Resource=flinksessionjobs" that match label selector "", field selector "spec.deploymentName=basic-session-cluster": field label not supported: spec.deploymentName
>
> kubectl version --short
> Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
> Client Version: v1.24.0
> Kustomize Version: v4.5.4
> Server Version: v1.23.3
> ```
>
> After search the related issues, I think the custom resource can only support field selector by `metadata.namespace` and `metadata.name`
>
> [kubernetes/kubernetes#53345](https://github.com/kubernetes/kubernetes/pull/53345)
> > k get sessionjob --field-selector spec.deploymentName=basic-session-cluster
>
> I tried this way before, but the custom resource seems do not support field selector
>
> ```
> kubectl get sessionjob --field-selector spec.deploymentName=basic-session-cluster
> Error from server (BadRequest): Unable to find "flink.apache.org/v1beta1, Resource=flinksessionjobs" that match label selector "", field selector "spec.deploymentName=basic-session-cluster": field label not supported: spec.deploymentName
>
> kubectl version --short
> Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
> Client Version: v1.24.0
> Kustomize Version: v4.5.4
> Server Version: v1.23.3
> ```
>
> After search the related issues, I think the custom resource can only support field selector by `metadata.namespace` and `metadata.name`
>
> [kubernetes/kubernetes#53345](https://github.com/kubernetes/kubernetes/pull/53345)
Yeah, I was almost sure you've tried it, but I thought it was worth asking :)
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873107836
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
Got it, but this is the first use case that we modify something else then the status. We might need to reconsider this again. Updating the CR deep in the stack sounds worse to me.
--
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] morhidi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126903507
> TBH, I do not like to do the CR `.metadata` or `.spec` updates in the reconciler. Especially, this is only for easier filter, not for operator internal use case. In the production environment, I believe the users could do this manually or covered by external job management tools.
>
> I also have another feeling this is the responsibility of [mutating admission webhook](https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#mutatingadmissionwebhook).
I've been also thinking about putting this into the webhook or maybe using the label exclusively for linking the two CRs.
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r872986702
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
Review Comment:
I suggest to extract adding labels into a utility method to keep the reconcile method concise
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r872986702
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
Review Comment:
I suggest to extract adding labels into a utility method (we could use computeIfAbsent)
--
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] Aitozi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1141045169
closing it now
--
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] morhidi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873110405
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
We hit some race conditions anyway with the status updates, so it maybe good as is with the java-operator-sdk v2 and we can change it later.
--
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 #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873195809
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
if you don't explicitly set the resourceversion to `null` before patching you might still get an error.
--
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] Aitozi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1127144890
Thanks for your good suggestion @morhidi @wangyang0918 @gyfora . I will try to implement it with mutating webhook. It's ok to me to leave it for next release
--
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] Aitozi commented on pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1126848686
> LGTM overall. nit: Shouldn't we use a shorter label, e.g:
>
> ```
> k get sessionjob -l target=basic-session-cluster
> ```
>
> It's easier to remember :)
Good idea, but the `target` is a bit too generic, I lean to rename it to `target.session`
--
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] Aitozi commented on a diff in pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#discussion_r873281386
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##########
@@ -76,6 +78,24 @@ public void reconcile(FlinkSessionJob flinkSessionJob, Context context) throws E
OperatorUtils.getSecondaryResource(
flinkSessionJob, context, configManager.getOperatorConfiguration());
+ var labels = flinkSessionJob.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new HashMap<>();
+ }
+ if (!flinkSessionJob
+ .getSpec()
+ .getDeploymentName()
+ .equals(labels.get(LABEL_SESSION_CLUSTER))) {
+ labels.put(LABEL_SESSION_CLUSTER, flinkSessionJob.getSpec().getDeploymentName());
+ flinkSessionJob.getMetadata().setLabels(labels);
+ kubernetesClient
Review Comment:
yeah, but I think this operation is retriable
--
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 pull request #215: [FLINK-27163] Add label for the session job to help list the session …
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #215:
URL: https://github.com/apache/flink-kubernetes-operator/pull/215#issuecomment-1140852188
Should we close this PR for now?
--
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