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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
