[GitHub] [flink-kubernetes-operator] mxm commented on a diff in pull request #533: [FLINK-30920] [AutoScaler] adds exclude vertex ids for autoscaler to ignore for evaluation and scaling

["mxm (via GitHub)" <gi...@apache.org>]

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


##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/config/AutoScalerOptions.java:
##########
@@ -153,4 +153,10 @@ private static ConfigOptions.OptionBuilder autoScalerConfig(String key) {
                     .defaultValue(MetricAggregator.MAX)
                     .withDescription(
                             "Metric aggregator to use for busyTime metrics. This affects how true processing/output rate will be computed. Using max allows us to handle jobs with data skew more robustly, while avg may provide better stability when we know that the load distribution is even.");
+
+    public static final ConfigOption<String> EXCLUDE_VERTEX_IDS =
+            autoScalerConfig("exclude.vertex.ids")
+                    .stringType()

Review Comment:
   This should be of list type.
   
   ```suggestion
                       .asList()
   ```



##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/config/AutoScalerOptions.java:
##########
@@ -153,4 +153,10 @@ private static ConfigOptions.OptionBuilder autoScalerConfig(String key) {
                     .defaultValue(MetricAggregator.MAX)
                     .withDescription(
                             "Metric aggregator to use for busyTime metrics. This affects how true processing/output rate will be computed. Using max allows us to handle jobs with data skew more robustly, while avg may provide better stability when we know that the load distribution is even.");
+
+    public static final ConfigOption<String> EXCLUDE_VERTEX_IDS =
+            autoScalerConfig("exclude.vertex.ids")
+                    .stringType()
+                    .defaultValue("")
+                    .withDescription("comma separated list of vertex ids in hexstring to disable for autoscaler to ignore for evaluating and scaling.");

Review Comment:
   This would be a semicolon-separated list in case of ListType.



##########
flink-kubernetes-operator-autoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ScalingMetricEvaluator.java:
##########
@@ -66,12 +67,15 @@ public Map<JobVertexID, Map<ScalingMetric, EvaluatedScalingMetric>> evaluate(
         var scalingOutput = new HashMap<JobVertexID, Map<ScalingMetric, EvaluatedScalingMetric>>();
         var metricsHistory = collectedMetrics.getMetricHistory();
         var topology = collectedMetrics.getJobTopology();
+        var excludeVertexIdList = Arrays.asList(conf.get(AutoScalerOptions.EXCLUDE_VERTEX_IDS).split(","));

Review Comment:
   No need to do this here if we use a list type like other options, e.g.  see `classloader.parent-first-patterns.default`.



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