gyfora commented on code in PR #622:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/622#discussion_r1239818902
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkDeploymentController.java:
##########
@@ -140,6 +141,12 @@ public UpdateControl<FlinkDeployment>
reconcile(FlinkDeployment flinkApp, Contex
previousDeployment,
false);
}
+ // Rely on the last stable spec if rolling back
+ if (flinkApp.getStatus().getReconciliationStatus().getState()
+ == ReconciliationState.ROLLING_BACK) {
+ flinkApp.setSpec(
+
flinkApp.getStatus().getReconciliationStatus().deserializeLastStableSpec());
Review Comment:
We might want to do a spec diff check there with the current spec vs the
last reconciled spec. It could happen that we initiated the rollback (setting
the state to ROLLING_BACK) and in the meantime the user actually submitted a
new spec.
With the current logic the new spec would be first ignored and we perform a
rollback and after that we reconcile the new spec. This is not entirely
incorrect but it will lead to extra time and extra redeployments.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconciliationUtils.java:
##########
@@ -125,12 +126,19 @@ private static <SPEC extends AbstractFlinkSpec> void
updateStatusForSpecReconcil
// Clear errors
status.setError(null);
reconciliationStatus.setReconciliationTimestamp(clock.instant().toEpochMilli());
- reconciliationStatus.setState(
- upgrading ? ReconciliationState.UPGRADING :
ReconciliationState.DEPLOYED);
+ var state = upgrading ? ReconciliationState.UPGRADING :
ReconciliationState.DEPLOYED;
+ if (status.getReconciliationStatus().getState() ==
ReconciliationState.ROLLING_BACK) {
+ state = upgrading ? ReconciliationState.ROLLING_BACK :
ReconciliationState.ROLLED_BACK;
+ }
+ reconciliationStatus.setState(state);
+ var clonedSpec = ReconciliationUtils.clone(spec);
+ if (status.getReconciliationStatus().getState() ==
ReconciliationState.ROLLING_BACK
+ || status.getReconciliationStatus().getState() ==
ReconciliationState.ROLLED_BACK) {
+ clonedSpec = reconciliationStatus.deserializeLastReconciledSpec();
+ }
Review Comment:
Both of these if branches have some redundant code, setting the
state/clonedSpec variables twice instead of once in case of redeployments.
--
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]
