mxm commented on code in PR #921:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/921#discussion_r1879606901
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##########
@@ -89,13 +89,17 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
+ "seconds suffix, daily expression's
formation is startTime-endTime, such as 9:30:30-10:50:20, when exclude from
9:30:30-10:50:20 in Monday and Thursday "
+ "we can express it as 9:30:30-10:50:20
&& * * * ? * 2,5");
- public static final ConfigOption<Double> TARGET_UTILIZATION =
- autoScalerConfig("target.utilization")
+ public static final ConfigOption<Double> UTILIZATION_TARGET =
+ autoScalerConfig("utilization.target")
Review Comment:
Do we have to change this well-established key? This might confuse users,
even though we have a fallback.
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##########
@@ -89,13 +89,17 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
+ "seconds suffix, daily expression's
formation is startTime-endTime, such as 9:30:30-10:50:20, when exclude from
9:30:30-10:50:20 in Monday and Thursday "
+ "we can express it as 9:30:30-10:50:20
&& * * * ? * 2,5");
- public static final ConfigOption<Double> TARGET_UTILIZATION =
- autoScalerConfig("target.utilization")
+ public static final ConfigOption<Double> UTILIZATION_TARGET =
+ autoScalerConfig("utilization.target")
Review Comment:
I understand its cleaner, but perhaps we can simply add the new keys and
leave this unchanged?
##########
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricEvaluator.java:
##########
@@ -296,8 +297,16 @@ protected static void computeProcessingRateThresholds(
upperUtilization = 1.0;
lowerUtilization = 0.0;
} else {
- upperUtilization = targetUtilization + utilizationBoundary;
- lowerUtilization = targetUtilization - utilizationBoundary;
+ if (conf.getOptional(UTILIZATION_MAX).isPresent()
+ || conf.getOptional(UTILIZATION_MIN).isPresent()
+ ||
conf.getOptional(TARGET_UTILIZATION_BOUNDARY).isEmpty()) {
+ upperUtilization = conf.get(UTILIZATION_MAX);
+ lowerUtilization = conf.get(UTILIZATION_MIN);
Review Comment:
I'm wondering whether the logic should be the following?
```suggestion
if (conf.getOptional(UTILIZATION_MAX).isPresent()
&& conf.getOptional(UTILIZATION_MIN).isPresent()) {
upperUtilization = conf.get(UTILIZATION_MAX);
lowerUtilization = conf.get(UTILIZATION_MIN);
```
--
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]
