kfaraz commented on code in PR #18480:
URL: https://github.com/apache/druid/pull/18480#discussion_r2324294101
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java:
##########
@@ -212,6 +218,15 @@ public AggregateFunction getLagAggregate()
return lagAggregate;
}
+ @Override
+ @JsonProperty
+ @Nullable
+ @JsonInclude(JsonInclude.Include.NON_NULL)
Review Comment:
This can be moved to the class level itself.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java:
##########
@@ -100,6 +103,9 @@ public LagBasedAutoScalerConfig(
this.scaleOutStep = scaleOutStep != null ? scaleOutStep : 2;
this.minTriggerScaleActionFrequencyMillis =
minTriggerScaleActionFrequencyMillis
!= null ? minTriggerScaleActionFrequencyMillis : 600000;
+
+ Preconditions.checkArgument(stopTaskCountPercent == null ||
(stopTaskCountPercent > 0.0 && stopTaskCountPercent <= 1.0), "0.0 <
stopTaskCountPercent <= 1.0");
Review Comment:
If the value range is [0, 1], we should call the field `stopTaskCountRatio`.
Alternatively, if we want to keep a percentage in the range [0, 100], we
should use an int field instead.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -0,0 +1,294 @@
+/*
+ * 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;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import
org.apache.druid.indexing.seekablestream.supervisor.autoscaler.LagBasedAutoScalerConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class LagBasedAutoScalerConfigTest
Review Comment:
Thanks for adding these tests!
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java:
##########
@@ -49,6 +50,7 @@ public class LagBasedAutoScalerConfig implements
AutoScalerConfig
private final boolean enableTaskAutoScaler;
private final long minTriggerScaleActionFrequencyMillis;
private final AggregateFunction lagAggregate;
+ @Nullable private Double stopTaskCountPercent;
Review Comment:
Nit: I don't think we need to mark this as nullable too. It should be enough
to mark just the getter as nullable.
##########
docs/ingestion/supervisor.md:
##########
@@ -80,6 +80,7 @@ The following table outlines the configuration properties for
`autoScalerConfig`
|`taskCountStart`|Optional config to specify the number of ingestion tasks to
start with. When you enable the autoscaler, Druid ignores the value of
`taskCount` in `ioConfig` and, if specified, starts with the `taskCountStart`
number of tasks. Otherwise, defaults to `taskCountMin`.|No|`taskCountMin`|
|`minTriggerScaleActionFrequencyMillis`|The minimum time interval between two
scale actions.| No|600000|
|`autoScalerStrategy`|The algorithm of autoscaler. Druid only supports the
`lagBased` strategy. See [Autoscaler strategy](#autoscaler-strategy) for more
information.|No|`lagBased`|
+|`stopTaskCountPercent`|A variable version of `ioConfig.stopTaskCount`. Allows
the maximum number of stoppable tasks in steady state to be proportional to the
# of tasks running.|No||
Review Comment:
```suggestion
|`stopTaskCountPercent`|A variable version of `ioConfig.stopTaskCount`.
Allows the maximum number of stoppable tasks in steady state to be proportional
to the number of tasks currently running.|No||
```
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -0,0 +1,294 @@
+/*
+ * 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;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import
org.apache.druid.indexing.seekablestream.supervisor.autoscaler.LagBasedAutoScalerConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class LagBasedAutoScalerConfigTest
+{
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ @Test
+ public void testSerdeWithDefaults() throws Exception
+ {
+ LagBasedAutoScalerConfig config = OBJECT_MAPPER.readValue(
+ OBJECT_MAPPER.writeValueAsString(
+ OBJECT_MAPPER.readValue(
+ "{}",
+ LagBasedAutoScalerConfig.class
+ )
Review Comment:
Just this part should be enough to verify the default values.
We need not perform a repeated deserialize-serialize-deserialize.
##########
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java:
##########
@@ -2689,6 +2690,55 @@ public Duration getEmissionDuration()
replayAll();
}
+ @Test
+ public void testMaxAllowedStopsWithStopTaskCountPercent()
+ {
+ LagAggregator lagAggregator = createMock(LagAggregator.class);
+ AutoScalerConfig autoScalerConfig = new LagBasedAutoScalerConfig(
Review Comment:
Looking at the various test cases, I think it might make sense to add a
`Builder` for the `LagBasedAutoScalerConfig` class. Not a blocker for this PR,
though.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/autoscaler/LagBasedAutoScalerConfigTest.java:
##########
@@ -0,0 +1,294 @@
+/*
+ * 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;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import
org.apache.druid.indexing.seekablestream.supervisor.autoscaler.LagBasedAutoScalerConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class LagBasedAutoScalerConfigTest
+{
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ @Test
+ public void testSerdeWithDefaults() throws Exception
+ {
+ LagBasedAutoScalerConfig config = OBJECT_MAPPER.readValue(
+ OBJECT_MAPPER.writeValueAsString(
+ OBJECT_MAPPER.readValue(
+ "{}",
+ LagBasedAutoScalerConfig.class
+ )
Review Comment:
We can have one separate test which verifies that the object obtained on
serialize and then deserialize is the same as the original.
--
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]
