[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #165: [FLINK-26140] Support rollback strategies

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

bgeng777 commented on code in PR #165:
URL: https://github.com/apache/flink-kubernetes-operator/pull/165#discussion_r850061884


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/deployment/AbstractDeploymentObserver.java:
##########
@@ -159,16 +185,18 @@ private void checkCrashLoopBackoff(FlinkDeployment flinkApp, Configuration effec
         }
     }
 
-    protected boolean isClusterReady(FlinkDeployment dep) {
+    protected boolean isJmDeploymentReady(FlinkDeployment dep) {
         return dep.getStatus().getJobManagerDeploymentStatus() == JobManagerDeploymentStatus.READY;
     }
 
     protected void clearErrorsIfJobManagerDeploymentNotInErrorStatus(FlinkDeployment dep) {
-        if (dep.getStatus().getJobManagerDeploymentStatus() != JobManagerDeploymentStatus.ERROR) {
-            final ReconciliationStatus reconciliationStatus =
-                    dep.getStatus().getReconciliationStatus();
-            reconciliationStatus.setSuccess(true);
-            reconciliationStatus.setError(null);
+        FlinkDeploymentStatus status = dep.getStatus();
+        ReconciliationStatus reconciliationStatus = status.getReconciliationStatus();
+        if (status.getJobManagerDeploymentStatus() != JobManagerDeploymentStatus.ERROR
+                && reconciliationStatus
+                        .getLastReconciledSpec()
+                        .equals(reconciliationStatus.getLastStableSpec())) {

Review Comment:
   nit: looks like we can directly use `reconciliationStatus.isLastReconciledSpecStable`.
   
   Besides, I want to make sure I under the new condition correctly: previously, we will try to clear errors and once the JM deployment is not in ERROR status, we do the cleaning. After introducing the `isLastReconciledSpecStable` check, we only `markReconciledSpecAsStable` when `isJmDeploymentReady` && `observeFlinkCluster` && `reconciliationStatus.getState() != ReconciliationStatus.State.ROLLED_BACK`. As a result, if JM is in states like `DEPLOYED_NOT_READY`, the error in status will not be cleared.  
   It seems that the later clear will not influence the correctness of the current logic as whether error info is null is only checked in `isJobUpgradeInProgress` but may make the method name `clearErrorsIfJobManagerDeploymentNotInErrorStatus` misleading. WDYT?



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