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


##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##########
@@ -149,19 +149,31 @@ public CollectedMetricHistory updateMetrics(
         // Add scaling metrics to history if they were computed successfully
         metricHistory.put(now, scalingMetrics);
 
-        if (isStabilizing) {
-            LOG.info("Stabilizing until {}", readable(stableTime));
-            stateStore.storeCollectedMetrics(ctx, metricHistory);
-            return new CollectedMetricHistory(topology, 
Collections.emptySortedMap(), jobRunningTs);
-        }
-
         var collectedMetrics = new CollectedMetricHistory(topology, 
metricHistory, jobRunningTs);
         if (now.isBefore(windowFullTime)) {
-            LOG.info("Metric window not full until {}", 
readable(windowFullTime));
+            if (isStabilizing) {
+                LOG.info(
+                        "Stabilizing... until {}. {} samples collected",

Review Comment:
   True, we return the metrics from the stabilization period once the 
stabilization period is over. They are used to calculate the observed true 
processing rate. When the window is full, we clear the metrics from the 
stabilization period and mark the window as fully collected.
   
   I still think that returning this logging message ("XX samples collected") 
to the user during stabilization is confusing. It suggests that these metrics 
will be evaluated for scaling decisions, which they are not. We are merely 
measuring the processing capacity of the sources.



##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/utils/DateTimeUtils.java:
##########
@@ -26,7 +26,7 @@
 public class DateTimeUtils {
 
     private static final DateTimeFormatter DEFAULT_FORMATTER =
-            DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
+            DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS");

Review Comment:
   AFAIK we don't use any milliseconds in the tests, but advance only by full 
seconds. I think this may have unexpected consequences for the storage and 
logging layer. If this is a test-only change, then it should go into test.



-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to