kfaraz commented on code in PR #18958:
URL: https://github.com/apache/druid/pull/18958#discussion_r2758022407
##########
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/CostBasedAutoScalerConfigTest.java:
##########
@@ -81,10 +93,15 @@ public void testSerdeWithDefaults() throws Exception
Assert.assertEquals(2, config.getTaskCountMin());
// Check defaults
- Assert.assertEquals(900000L, config.getScaleActionPeriodMillis());
- Assert.assertEquals(1200000L,
config.getMinTriggerScaleActionFrequencyMillis());
- Assert.assertEquals(0.25, config.getLagWeight(), 0.001);
- Assert.assertEquals(0.75, config.getIdleWeight(), 0.001);
+ Assert.assertEquals(DEFAULT_SCALE_ACTION_PERIOD_MILLIS,
config.getScaleActionPeriodMillis());
+ Assert.assertEquals(DEFAULT_MIN_TRIGGER_SCALE_ACTION_FREQUENCY_MILLIS,
config.getMinTriggerScaleActionFrequencyMillis());
+ Assert.assertEquals(DEFAULT_LAG_WEIGHT, config.getLagWeight(), 0.001);
+ Assert.assertEquals(DEFAULT_IDLE_WEIGHT, config.getIdleWeight(), 0.001);
+ Assert.assertEquals(DEFAULT_PROCESSING_RATE,
config.getDefaultProcessingRate(), 0.001);
+ Assert.assertEquals(DEFAULT_SCALE_DOWN_BARRIER,
config.getScaleDownBarrier());
Review Comment:
Rather than importing the constants, it would be nice to verify the
numerical values directly here, so that when the constants change, we
consciously update the tests as well.
##########
docs/ingestion/supervisor.md:
##########
@@ -208,6 +208,8 @@ The following table outlines the configuration properties
related to the `costBa
|`lagWeight`|The weight of extracted lag value in cost function.| No| 0.25|
|`idleWeight`|The weight of extracted poll idle value in cost function. | No |
0.75 |
|`defaultProcessingRate`|A planned processing rate per task, required for
first cost estimations. | No | 1000 |
+|`scaleDownBarrier`| A number of successful scale down attempts which should
be skipped to prevent the auto-scaler from scaling down tasks immediately. |
No | 5 |
Review Comment:
This config does not seem to be aligned with the whole cost-based story.
It is a little counterintuitive as it essentially negates the decision taken
by the auto-scaler.
IIUC, this has been put in as a hack until we have fixed the issue with
scaling on task rollover.
Please remove this config and we should try to evaluate the following
instead:
- increase the window over which we compute the average lag
- AND/OR add more terms to the cost function
- AND/OR add a config like `minScaleDownDelay` (which serves a role similar
to the `scaleDownBarrier` but in terms of time rather than count)
##########
docs/ingestion/supervisor.md:
##########
@@ -208,6 +208,8 @@ The following table outlines the configuration properties
related to the `costBa
|`lagWeight`|The weight of extracted lag value in cost function.| No| 0.25|
|`idleWeight`|The weight of extracted poll idle value in cost function. | No |
0.75 |
|`defaultProcessingRate`|A planned processing rate per task, required for
first cost estimations. | No | 1000 |
+|`scaleDownBarrier`| A number of successful scale down attempts which should
be skipped to prevent the auto-scaler from scaling down tasks immediately. |
No | 5 |
+|`scaleDownDuringTaskRolloverOnly`| Indicates whether task scaling down is
limited to periods during task rollovers only. | No | False |
Review Comment:
Thanks for adding this config, it makes sense to have this option. But I
agree with @capistrant in that we would want the default of this flag to be
`true` so that scale down happens only on task rollover by default. But we can
fix it up once we have addressed the issues with the task rollover bit.
--
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]
