kfaraz commented on code in PR #16334:
URL: https://github.com/apache/druid/pull/16334#discussion_r1581717506
##########
docs/ingestion/supervisor.md:
##########
@@ -96,6 +96,7 @@ The following table outlines the configuration properties
related to the `lagBas
|`scaleActionPeriodMillis`|The frequency in milliseconds to check if a scale
action is triggered.|No|60000|
|`scaleInStep`|The number of tasks to reduce at once when scaling down.|No|1|
|`scaleOutStep`|The number of tasks to add at once when scaling out.|No|2|
+|`lagStatsType`|The stat ("MAX"/"TOTAL"/"AVG") to choose from the partitions
lag for scaling decisions|No|Default provided by the supervisor|
Review Comment:
Currently the default value is SUM for all supervisors. We can update the
docs when we have different defaults for different supervisors.
```suggestion
|`lagAggregate`|The aggregate function used to compute the lag metric for
scaling decisions. Possible values are `MAX`, `SUM` and `AVERAGE`. |No|`SUM`|
```
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/ScalingMetric.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord.supervisor.autoscaler;
+
+public enum ScalingMetric
+{
+ MAX,
+ TOTAL,
Review Comment:
```suggestion
SUM,
```
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/ScalingMetric.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord.supervisor.autoscaler;
+
+public enum ScalingMetric
Review Comment:
I think a better name would be `AggregateFunction`.
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagStats.java:
##########
@@ -46,4 +53,22 @@ public long getAvgLag()
{
return avgLag;
}
+
+ public long getPrefferedScalingMetric()
Review Comment:
Typo. This method should return the `ScalingMetric` object itself.
```suggestion
public ScalingMetric getPreferredScalingMetric()
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java:
##########
@@ -61,7 +63,8 @@ public LagBasedAutoScalerConfig(
@Nullable @JsonProperty("scaleInStep") Integer scaleInStep,
@Nullable @JsonProperty("scaleOutStep") Integer scaleOutStep,
@Nullable @JsonProperty("enableTaskAutoScaler") Boolean
enableTaskAutoScaler,
- @Nullable @JsonProperty("minTriggerScaleActionFrequencyMillis") Long
minTriggerScaleActionFrequencyMillis
+ @Nullable @JsonProperty("minTriggerScaleActionFrequencyMillis") Long
minTriggerScaleActionFrequencyMillis,
+ @Nullable @JsonProperty("lagStatsType") ScalingMetric lagStatsType
Review Comment:
Suggestion for rename:
```suggestion
@Nullable @JsonProperty("lagAggregate") AggregateFunction
lagAggregate
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScaler.java:
##########
@@ -154,8 +155,15 @@ private Runnable computeAndCollectLag()
LOCK.lock();
try {
if (!spec.isSuspended()) {
- long lag = supervisor.computeLagForAutoScaler();
- lagMetricsQueue.offer(lag > 0 ? lag : 0L);
+ LagStats lagStats = supervisor.computeLagStats();
+ if (lagStats != null) {
+ long lag = lagBasedAutoScalerConfig.getLagStatsType() != null ?
+
lagStats.getMetric(lagBasedAutoScalerConfig.getLagStatsType()) :
+ lagStats.getPrefferedScalingMetric();
Review Comment:
```suggestion
final ScalingMetric scalingMetric =
lagBasedAutoScalerConfig.getLagStatsType() == null
?
lagStats.getPreferredScalingMetric()
:
lagBasedAutoScalerConfig.getLagStatsType() == null;
long lag = lagStats.getMetric(scalingMetric);
```
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagStats.java:
##########
@@ -24,12 +24,19 @@ public class LagStats
private final long maxLag;
private final long totalLag;
private final long avgLag;
+ private final ScalingMetric preferredScalingMetric;
Review Comment:
Nit: Thinking about the name again, we could call this field
`preferredAggregateForScaling` or even simply `aggregateForScaling`.
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagStats.java:
##########
@@ -24,12 +24,19 @@ public class LagStats
private final long maxLag;
private final long totalLag;
private final long avgLag;
+ private final ScalingMetric preferredScalingMetric;
public LagStats(long maxLag, long totalLag, long avgLag)
+ {
+ this(maxLag, totalLag, avgLag, ScalingMetric.TOTAL);
+ }
+
+ public LagStats(long maxLag, long totalLag, long avgLag, ScalingMetric
preferredScalingMetric)
{
this.maxLag = maxLag;
this.totalLag = totalLag;
this.avgLag = avgLag;
+ this.preferredScalingMetric = preferredScalingMetric;
Review Comment:
We should assign it a default value of `SUM` if the passed argument is null.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
