suneet-s commented on code in PR #16334:
URL: https://github.com/apache/druid/pull/16334#discussion_r1580183635
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java:
##########
@@ -61,7 +62,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("useDefaultTotalLag") Boolean
useDefaultTotalLag
Review Comment:
I think it would be better to have this be an enum, like what was done in
the initial patch. Then the docs for this field would tell the user to set this
to one of `TOTAL`, `MAX` or `AVG` with the default implementation using the
total lag.
@kfaraz do you have any concerns with this approach since you had some
comments about the use of an enum in an earlier patch.
On naming - I think the name could be something like `lagStatsType` The word
`default` in the current name is confusing to me. Once there is agreement on
the approach, there should also be a doc update in the `supervisor.md` page for
this new config option.
--
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]
