gyfora commented on code in PR #928:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/928#discussion_r1908640936


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractFlinkResourceReconciler.java:
##########
@@ -185,11 +185,26 @@ private Optional<String> 
getInitialSnapshotPath(AbstractFlinkSpec spec) {
 
     private void applyAutoscaler(FlinkResourceContext<CR> ctx) throws 
Exception {
         var autoScalerCtx = ctx.getJobAutoScalerContext();
+        var resource = ctx.getResource();
         boolean autoscalerEnabled =
-                ctx.getResource().getSpec().getJob() != null
+                resource.getSpec().getJob() != null
                         && 
ctx.getObserveConfig().getBoolean(AUTOSCALER_ENABLED);
         autoScalerCtx.getConfiguration().set(AUTOSCALER_ENABLED, 
autoscalerEnabled);
 
+        var reconStatus = resource.getStatus().getReconciliationStatus();
+        if (!reconStatus.isBeforeFirstDeployment() && autoscalerEnabled) {
+            var newResetNonce = 
resource.getSpec().getJob().getAutoscalerResetNonce();
+            // check if the nonce changed to a non-null value
+            if (newResetNonce != null
+                    && !newResetNonce.equals(
+                            reconStatus
+                                    .deserializeLastReconciledSpec()
+                                    .getJob()
+                                    .getAutoscalerResetNonce())) {
+                autoscaler.cleanup(autoScalerCtx.getJobKey());

Review Comment:
   The parallelism overrides do not need to be removed as the last applied spec 
change is not stored in kubernetes. so what is in the spec at this point is 
what the user provided originally.
   
   As for the cleanup, you are right but I think this may actually be a 
problem/bug with the cleanup logic.
   
   I think cleanup (which we also call when deleting a custom resource) should 
probably delete the actual data from the state store as well. In kubernetes 
this works because the configmap is deleted automatically but as you suggest 
with the JDBC state store we would leak the autoscaler data after cleanup



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

Reply via email to