gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433757961
##########
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 don't completely understand why we had to move this into a method. It
could simply be:
```
var scaleEnabled = conf.get(SCALING_ENABLED) &&
!CalendarUtils.inExcludedPeriods(conf, now);
autoScalerEventHandler.handleScalingEvent(
context,
scalingSummaries,
scaleEnabled,
conf.get(SCALING_EVENT_INTERVAL));
```
Then we don't need any changes to the autoscaler event handler, we could
simply log a line here that we are in an excluded period instead of changing
code in multiple places.
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/event/AutoScalerEventHandler.java:
##########
@@ -83,12 +92,27 @@ default void handleScalingEvent(
}
static String scalingReport(
- Map<JobVertexID, ScalingSummary> scalingSummaries, boolean
scalingEnabled) {
- StringBuilder sb =
- new StringBuilder(
- scalingEnabled
- ? SCALING_SUMMARY_HEADER_SCALING_ENABLED
- : SCALING_SUMMARY_HEADER_SCALING_DISABLED);
+ Map<JobVertexID, ScalingSummary> scalingSummaries,
+ boolean scalingEnabled,
+ boolean isExcluded,
+ Configuration config) {
+ StringBuilder sb = new StringBuilder();
+ if (!scalingEnabled) {
+ sb.append(
+ String.format(
+ SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+ SCALING_ENABLED.key(),
+ false));
+ } else if (isExcluded) {
+ sb.append(
+ String.format(
+ SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+ EXCLUDED_PERIODS.key(),
+ config.get(EXCLUDED_PERIODS)));
+ } else {
+ sb.append(SCALING_SUMMARY_HEADER_SCALING_EXECUTION_ENABLED);
+ }
Review Comment:
as I wrote in the other comment, I prefer not to make any changes in the
event handler
--
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]
