jihoonson commented on a change in pull request #11440:
URL: https://github.com/apache/druid/pull/11440#discussion_r669763507
##########
File path:
extensions-contrib/gce-extensions/src/main/java/org/apache/druid/indexing/overlord/autoscaling/gce/GceAutoScaler.java
##########
@@ -80,8 +80,8 @@ public GceAutoScaler(
@JsonProperty("envConfig") GceEnvironmentConfig envConfig
)
{
- Preconditions.checkArgument(minNumWorkers > 0,
- "minNumWorkers must be greater than 0");
+ Preconditions.checkArgument(minNumWorkers >= 0,
Review comment:
Ah sure, I didn't mean that the check code should be in each autoScaler
because it's not possible as autoScaler doesn't know about
provisioningStrategyConfig. The check code should be in
`PendingTaskBasedWorkerProvisioningStrategy` instead. I just didn't find a good
place to leave my comment earlier :sweat_smile:
> However, this means that the failure would only show when the auto scaler
cycle runs. I think this is ok as the
PendingTaskBasedWorkerProvisioningStrategy#getDefaultWorkerBehaviorConfig
already have some check that make sure the config/AutoScaler set is valid.
Hmm, the check can throw an exception in the constructor of
`PendingTaskBasedWorkerProvisioningStrategy`, can't it? Because the constructor
accepts both `WorkerBehaviorConfig` and
`PendingTaskBasedWorkerProvisioningConfig` as its parameter.
--
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]
