gyfora commented on a change in pull request #56:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/56#discussion_r825658518
##########
File path:
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/Observer.java
##########
@@ -96,23 +104,24 @@ private void observeJmDeployment(
if (status != null
&& status.getAvailableReplicas() != null
&& spec.getReplicas().intValue() == status.getReplicas()
- && spec.getReplicas().intValue() ==
status.getAvailableReplicas()
- && flinkService.isJobManagerPortReady(effectiveConfig)) {
-
- // typically it takes a few seconds for the REST server to be
ready
+ && spec.getReplicas().intValue() ==
status.getAvailableReplicas()) {
Review comment:
Could we please move these 4 conditions into a `deploymentExists` or
something similar method to simplify the flow?
##########
File path:
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/Observer.java
##########
@@ -96,23 +104,24 @@ private void observeJmDeployment(
if (status != null
&& status.getAvailableReplicas() != null
&& spec.getReplicas().intValue() == status.getReplicas()
- && spec.getReplicas().intValue() ==
status.getAvailableReplicas()
- && flinkService.isJobManagerPortReady(effectiveConfig)) {
-
- // typically it takes a few seconds for the REST server to be
ready
+ && spec.getReplicas().intValue() ==
status.getAvailableReplicas()) {
+ if (flinkService.isJobManagerPortReady(effectiveConfig)) {
+ // typically it takes a few seconds for the REST server to
be ready
+ LOG.info(
+ "JobManager deployment {} in namespace {} port
ready, waiting for the REST API...",
+ flinkApp.getMetadata().getName(),
+ flinkApp.getMetadata().getNamespace());
+ deploymentStatus.setJobManagerDeploymentStatus(
+ JobManagerDeploymentStatus.DEPLOYED_NOT_READY);
+ return;
+ }
+ } else {
Review comment:
We could also remove the else branch here (as we return early in other
cases) to flatten the logic a bit to make it more readable.
##########
File path:
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/Observer.java
##########
@@ -96,23 +104,24 @@ private void observeJmDeployment(
if (status != null
&& status.getAvailableReplicas() != null
&& spec.getReplicas().intValue() == status.getReplicas()
- && spec.getReplicas().intValue() ==
status.getAvailableReplicas()
- && flinkService.isJobManagerPortReady(effectiveConfig)) {
-
- // typically it takes a few seconds for the REST server to be
ready
+ && spec.getReplicas().intValue() ==
status.getAvailableReplicas()) {
+ if (flinkService.isJobManagerPortReady(effectiveConfig)) {
Review comment:
I don't really see the point in moving this into a nested if branch with
fall-through logic. I think the simple if-else before was correct.
##########
File path:
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/Observer.java
##########
@@ -127,14 +136,26 @@ private void observeJmDeployment(
return;
}
}
+ // checking the pod is expensive; we only do it when the
deployment isn't ready
+ PodList jmPods = flinkService.getJmPodList(flinkApp,
effectiveConfig);
+ for (Pod pod : jmPods.getItems()) {
+ for (ContainerStatus cs :
pod.getStatus().getContainerStatuses()) {
+ ContainerStateWaiting csw = cs.getState().getWaiting();
+ if ("CrashLoopBackOff".equals(csw.getReason())) {
+ LOG.warn("JobManager pod fails: {} {}",
csw.getReason(), csw.getMessage());
+ return;
Review comment:
Could we move this check to a separate 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
