[GitHub] [flink-kubernetes-operator] gyfora commented on a change in pull request #21: [FLINK-26261] Wait for JobManager deployment ready before accessing REST API

[GitBox <gi...@apache.org>]

gyfora commented on a change in pull request #21:
URL: https://github.com/apache/flink-kubernetes-operator/pull/21#discussion_r813558909



##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkDeploymentController.java
##########
@@ -88,17 +92,23 @@ public DeleteControl cleanup(FlinkDeployment flinkApp, Context context) {
                 operatorNamespace,
                 kubernetesClient,
                 true);
+        jobManagerDeployments.remove(flinkApp);

Review comment:
       Should we also remove from this map when we cancel the flink job in the Flink service?

##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkDeploymentController.java
##########
@@ -109,6 +119,22 @@ public DeleteControl cleanup(FlinkDeployment flinkApp, Context context) {
         } catch (Exception e) {
             throw new ReconciliationException(e);
         }
+
+        if (!jobManagerDeployments.containsKey(flinkApp)) {
+            return context.getSecondaryResource(Deployment.class)
+                    .map(
+                            deployment -> {
+                                LOG.info(
+                                        "JobManager deployment {} in namespace {} is ready",
+                                        flinkApp.getMetadata().getName(),
+                                        flinkApp.getMetadata().getNamespace());
+                                jobManagerDeployments.put(flinkApp, deployment);

Review comment:
       Shouldn't we check and put into the jobManagerDeployments befre we try to observe? It seems now we introduce an extra refesh between every reconcile and observe.

##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkDeploymentController.java
##########
@@ -88,17 +92,23 @@ public DeleteControl cleanup(FlinkDeployment flinkApp, Context context) {
                 operatorNamespace,
                 kubernetesClient,
                 true);
+        jobManagerDeployments.remove(flinkApp);

Review comment:
       Otherwise after an upgrade the observer might hit the same issue




-- 
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