[GitHub] [flink-kubernetes-operator] mxm commented on a diff in pull request #492: [FLINK-30463] Start stabilization period only after job goes into RUNNING

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

mxm commented on code in PR #492:
URL: https://github.com/apache/flink-kubernetes-operator/pull/492#discussion_r1054407083


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ScalingExecutor.java:
##########
@@ -121,13 +122,22 @@ public boolean scaleResource(
 
     private boolean stabilizationPeriodPassed(
             AbstractFlinkResource<?, ?> resource, Configuration conf) {
-        var now = clock.instant();
+        var jobStatus = resource.getStatus().getJobStatus();
+
+        if (!JobStatus.RUNNING.name().equals(jobStatus.getState())) {

Review Comment:
   I noticed while implementing that there are various invariants enforced in different layers. This per se is not a problem if they weren't leaky, i.e. we wouldn't pass the same data structures with different invariants through the layers. This becomes apparent in unit tests where implied invariants have to be modeled. One way to solve this would be to pass a different data structure, e.g. not pass JobStatus to lower levels but only the update time / running time.
   
   I think the check here doesn't hurt as it asserts an important invariant.



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