mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433989601
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##########
@@ -264,4 +262,21 @@ private static Map<String, String>
getVertexParallelismOverrides(
});
return overrides;
}
+
+ private boolean blockScalingExecution(
+ Context context,
+ Map<JobVertexID, ScalingSummary> scalingSummaries,
+ Configuration conf,
+ Instant now) {
+ var scaleEnabled = conf.get(SCALING_ENABLED);
+ var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+ autoScalerEventHandler.handleScalingEvent(
+ context,
+ scalingSummaries,
+ scaleEnabled,
+ isExcluded,
+ conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I was contemplating to comment on this as well, but after looking at the
code, there is some benefit to hinting whether scaling is disabled or blocked.
By treating blocked scaling as "disabled", we yield less information to the
user. I think it will be beneficial to learn WHY the scaling is disabled, i.e.
scaling disabled or scaling blocked.
>The event handler really has nothing to do with this feature (and many
other things that happen in the scaling executor)
Not sure about that. Event handlers are supposed to process events and in
the context of the autoscaler, a blocked scaling decision is a type of event we
may present to the user. We are already leaking whether scaling is enabled or
not to the event handler, why not also annotate why it is disabled?
--
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
