You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/02/11 11:31:37 UTC
[GitHub] [druid] sascha-coenen opened a new pull request #9350: Overlord can
support many autoscalers of different categories
sascha-coenen opened a new pull request #9350: Overlord can support many autoscalers of different categories
URL: https://github.com/apache/druid/pull/9350
Fixes #8695.
### Description
Added support of many autoscalers of different categories to serve tasks of according category.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/incubator-druid/blob/master/licenses.yaml)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `CategoriedWorkerBehaviorConfig`
* `PendingTaskBasedWorkerProvisioningStrategy`
* `SimpleWorkerProvisioningStrategy`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387251212
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
That means, users who want to use `autoScalers` can't use the console UI to set the spec and would instead have to manually send the http request to coordinator using or curl or whatever.
after this PR is merged, an issue should be created for console UI updates to make it possible to add `autoScalers` .
In this PR, we need to make sure that we stay totally backwards compatible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on issue #9350: Overlord to support
autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#issuecomment-599781142
This pull request **introduces 1 alert** when merging eb3a001f67254fd5e4532e5b174391fcae4ace99 into 09600db8f2bedd6965f15d8d7fe81b39bcd89ac9 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-dab8ee7a631ed210b7687c95ac2566baa1fd4f1c)
**new alerts:**
* 1 for Dereferenced variable may be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r382888648
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
I think things will be simplified if we removed this class and instead made `DefaultWorkerBehaviorConfig` have `List<AutoScaler> autoScalers` with constructor
```
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
@JsonProperty("autoScaler") AutoScaler autoScaler,
@JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
// fail if both "autoscalers" and "autoscaler" fields are non-null
this.autoScalers = autoScalers or Collections.singletonList(autoscaler);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] VladimirIordanov commented on a change in pull request
#9350: Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
VladimirIordanov commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r384977932
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
Ok. No objections. Let's move the `autoScalers` section to `DefaultWorkerBehaviorConfig` and remove `CategorizedWorkerBehaviorConfig`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387186422
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/ProvisioningUtil.java
##########
@@ -61,4 +78,96 @@ public boolean apply(ImmutableWorkerInfo worker)
};
}
+ @Nullable
+ public static DefaultWorkerBehaviorConfig getDefaultWorkerBehaviorConfig(
+ Supplier<WorkerBehaviorConfig> workerConfigRef,
+ String action
+ )
+ {
+ final WorkerBehaviorConfig workerBehaviorConfig = workerConfigRef.get();
+ if (workerBehaviorConfig == null) {
+ log.error("No workerConfig available, cannot %s workers.", action);
+ return null;
+ }
+ if (!(workerBehaviorConfig instanceof DefaultWorkerBehaviorConfig)) {
+ log.error(
+ "Only DefaultWorkerBehaviorConfig is supported as WorkerBehaviorConfig, [%s] given, cannot %s workers",
+ workerBehaviorConfig,
+ action
+ );
+ return null;
+ }
+ final DefaultWorkerBehaviorConfig workerConfig = (DefaultWorkerBehaviorConfig) workerBehaviorConfig;
+ if (workerConfig.getAutoScalers() == null) {
Review comment:
DefaultWorkerBehaviorConfig constructor shouldn't/doesn't allow instantiation with null autoScalers so this check is redundant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387845997
##########
File path: services/src/main/java/org/apache/druid/cli/CliOverlord.java
##########
@@ -353,6 +354,7 @@ private void configureOverlordHelpers(Binder binder)
}
/**
+ *
Review comment:
nit: not sure why this file changed at all :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387183295
##########
File path: docs/configuration/index.md
##########
@@ -990,6 +985,22 @@ To view last <n> entries of the audit history of worker config issue a GET reque
http://<OVERLORD_IP>:<port>/druid/indexer/v1/worker/history?count=<n>
```
+##### Default Worker Config
+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`selectStrategy`|How to assign tasks to MiddleManagers. Choices are `fillCapacity`, `equalDistribution`, and `javascript`.|equalDistribution|
+|`autoScaler`|Only used if autoscaling is enabled. See below.|null|
+
+##### Categorized Worker Config
Review comment:
this section should be removed and `autoScaler` be replaced with `autoScalers` above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on issue #9350: Overlord can
support many autoscalers of different categories
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on issue #9350: Overlord can support many autoscalers of different categories
URL: https://github.com/apache/druid/pull/9350#issuecomment-584707464
Just FYI: we had to move this code contribution from one github repo to another one today.
So we removed the pevious PR that had not been reviewed yet with this one. No code has changed, its just a new PR that originates from a different repo now.
Sorry for the inconvenience.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387812651
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/ProvisioningUtil.java
##########
@@ -61,4 +78,96 @@ public boolean apply(ImmutableWorkerInfo worker)
};
}
+ @Nullable
+ public static DefaultWorkerBehaviorConfig getDefaultWorkerBehaviorConfig(
+ Supplier<WorkerBehaviorConfig> workerConfigRef,
+ String action
+ )
+ {
+ final WorkerBehaviorConfig workerBehaviorConfig = workerConfigRef.get();
+ if (workerBehaviorConfig == null) {
+ log.error("No workerConfig available, cannot %s workers.", action);
+ return null;
+ }
+ if (!(workerBehaviorConfig instanceof DefaultWorkerBehaviorConfig)) {
+ log.error(
+ "Only DefaultWorkerBehaviorConfig is supported as WorkerBehaviorConfig, [%s] given, cannot %s workers",
+ workerBehaviorConfig,
+ action
+ );
+ return null;
+ }
+ final DefaultWorkerBehaviorConfig workerConfig = (DefaultWorkerBehaviorConfig) workerBehaviorConfig;
+ if (workerConfig.getAutoScalers() == null) {
+ log.error("No autoScaler available, cannot %s workers", action);
+ return null;
+ }
+ return workerConfig;
+ }
+
+ @Nullable
+ public static WorkerCategorySpec getWorkerCategorySpec(DefaultWorkerBehaviorConfig workerConfig)
+ {
+ if (workerConfig != null && workerConfig.getSelectStrategy() != null) {
+ WorkerSelectStrategy selectStrategy = workerConfig.getSelectStrategy();
+ if (selectStrategy instanceof CategorizedWorkerSelectStrategy) {
+ return ((CategorizedWorkerSelectStrategy) selectStrategy).getWorkerCategorySpec();
+ }
+ }
+ return null;
+ }
+
+ public static Map<String, AutoScaler> mapAutoscalerByCategory(List<AutoScaler> autoScalers)
+ {
+ Map<String, AutoScaler> result = autoScalers.stream().collect(Collectors.groupingBy(
+ ProvisioningUtil::getAutoscalerCategory,
+ Collectors.collectingAndThen(Collectors.toList(), values -> values.get(0))
+ ));
+
+ if (result.size() != autoScalers.size()) {
+ log.warn(
Review comment:
good point. I changed that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r395934763
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
+
+ didProvision = doProvision(
Review comment:
thanks for checking, I haven't looked that refactoring in details but I believe you.
for the antlr error to go away, do "mvn clean install -DskipTests" in `core` module whenever you switch branches.
> I am supposed to mark a conversation as resolved if I believe to have addressed a review comment or whether it is rather meant to be the reviewer's privilege to do that.
it is a new feature in github and I don't think there is any formal agreement in Druid community over this. But, I would say, PR author should feel free to mark the conversation resolved when they think that they addressed it. If Reviewer doesn't agree then they can mark it "unresolved" and conversation can be further resumed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r384862791
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
Actually, We are keeping `autoScaler` field only for backward compatibility .. we should really deprecate it and remove its mention from the docs. Eventually it should be removed and we should just have `autoScalers` .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387813003
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/DefaultWorkerBehaviorConfig.java
##########
@@ -23,29 +23,42 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
import org.apache.druid.indexing.overlord.autoscaling.NoopAutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
*/
public class DefaultWorkerBehaviorConfig implements WorkerBehaviorConfig
{
- private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler();
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+ private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler(DEFAULT_AUTOSCALER_CATEGORY);
public static DefaultWorkerBehaviorConfig defaultConfig()
{
- return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER);
+ return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER, null);
}
private final WorkerSelectStrategy selectStrategy;
- private final AutoScaler autoScaler;
+ private final List<AutoScaler> autoScalers;
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
- @JsonProperty("autoScaler") AutoScaler autoScaler
+ @JsonProperty("autoScaler") AutoScaler autoScaler,
+ @JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
- this.autoScaler = autoScaler;
+ this.autoScalers = (autoScaler != null) ? Collections.singletonList(autoScaler) : autoScalers;
+ if (this.autoScalers == null) {
+ throw new IllegalArgumentException("Either autoScaler or autoScalers property needs to be provided");
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] VladimirIordanov commented on a change in pull request
#9350: Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
VladimirIordanov commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r384353659
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
Do you thing it will not confuse users? I'm about having two sections related to autoscalers in the same config.
I'm ok for that. Initially I also had such decision to just extend DefaultWorkerBehaviorConfig. But later I thought that having two sections for autoscalers can be not good for user experience.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r393607284
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/WorkerSelectUtils.java
##########
@@ -108,8 +112,39 @@ public static ImmutableWorkerInfo selectWorker(
final Function<ImmutableMap<String, ImmutableWorkerInfo>, ImmutableWorkerInfo> workerSelector
)
{
- final Map<String, ImmutableWorkerInfo> runnableWorkers = getRunnableWorkers(task, allWorkers, workerTaskRunnerConfig);
+ final Map<String, ImmutableWorkerInfo> runnableWorkers = getRunnableWorkers(
+ task,
+ allWorkers,
+ workerTaskRunnerConfig
+ );
+
+ // select worker according to worker category spec
+ if (workerCategorySpec != null) {
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387198707
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerSelectStrategy.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.setup;
+
+/**
+ * Extended version of WorkerSelectStrategy which is suitable for categorized strategies.
+ * It acts as a marker interface used by CategorizedProvisioningStrategy to distinguish between legacy and categorized worker select strategies.
Review comment:
```suggestion
```
since all ProvisioningStrategy impl use this and CategorizedProvisioningStrategy doesn't exist
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387845217
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/WorkerSelectUtils.java
##########
@@ -108,8 +112,39 @@ public static ImmutableWorkerInfo selectWorker(
final Function<ImmutableMap<String, ImmutableWorkerInfo>, ImmutableWorkerInfo> workerSelector
)
{
- final Map<String, ImmutableWorkerInfo> runnableWorkers = getRunnableWorkers(task, allWorkers, workerTaskRunnerConfig);
+ final Map<String, ImmutableWorkerInfo> runnableWorkers = getRunnableWorkers(
+ task,
+ allWorkers,
+ workerTaskRunnerConfig
+ );
+
+ // select worker according to worker category spec
+ if (workerCategorySpec != null) {
Review comment:
this check is again done in getTaskCategory(..) and is redundant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r391180758
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
Review comment:
nit: replacement similar to other class could be used to avoid instantiation of `HashSet` here too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r384862500
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedWorkerProvisioningStrategy.java
##########
@@ -0,0 +1,659 @@
+/*
+ * 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.autoscaling;
+
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.inject.Inject;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.indexing.overlord.ImmutableWorkerInfo;
+import org.apache.druid.indexing.overlord.WorkerTaskRunner;
+import org.apache.druid.indexing.overlord.config.WorkerTaskRunnerConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.WorkerCategorySpec;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectUtils;
+import org.apache.druid.indexing.worker.Worker;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.concurrent.ScheduledExecutors;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+import org.joda.time.DateTime;
+import org.joda.time.Duration;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.stream.Collectors;
+
+/**
+ * Autoscaler provisioning strategy based on {@link AutoScaler#getCategory()} field. It selects autoscaler based on
+ * a worker's category.
+ */
+@JsonTypeName("categorizedTaskBased")
+public class CategorizedWorkerProvisioningStrategy extends AbstractWorkerProvisioningStrategy
Review comment:
yeah, that is what I guessed :)
categorization really cuts across all implementations of `ProvisioningStrategy` and hence shouldn't be a separate impl on its own.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r395933753
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/DefaultWorkerBehaviorConfig.java
##########
@@ -23,29 +23,52 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
import org.apache.druid.indexing.overlord.autoscaling.NoopAutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
*/
public class DefaultWorkerBehaviorConfig implements WorkerBehaviorConfig
{
- private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler();
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+ private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler(DEFAULT_AUTOSCALER_CATEGORY);
public static DefaultWorkerBehaviorConfig defaultConfig()
{
- return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER);
+ return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER, null);
}
private final WorkerSelectStrategy selectStrategy;
- private final AutoScaler autoScaler;
+ private final List<AutoScaler> autoScalers;
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
- @JsonProperty("autoScaler") AutoScaler autoScaler
+ @Deprecated @JsonProperty("autoScaler") AutoScaler autoScaler,
+ @JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
- this.autoScaler = autoScaler;
+ if (autoScaler != null && autoScalers != null) {
+ throw new IllegalArgumentException("The autoScaler and autoScalers properties are mutually exclusive");
+ }
+ if (autoScaler == null && autoScalers == null) {
+ throw new IllegalArgumentException("Either autoScaler or autoScalers property must be provided");
+ }
Review comment:
I agree, lgtm is just wrong in this case.
Also, I see that you are using `==` instead of `^`, while they both produce same result due to interning of boolean values I would prefer `^` which has clearly defined XOR meaning and reader for the code would get immediately context\. `==` is "tricky" way to do what `^` is doing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on issue #9350: Overlord to support
autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on issue #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#issuecomment-588945463
the spellchecker was failing on a naming issue, so I fixed the spelling in the documentation and also changed wrongly spelled type names in the code accordingly.
Now all checks have passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] VladimirIordanov commented on a change in pull request
#9350: Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
VladimirIordanov commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r384358764
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedProvisioningConfig.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.autoscaling;
+
+import org.joda.time.Period;
+
+public class CategorizedProvisioningConfig extends PendingTaskBasedWorkerProvisioningConfig
Review comment:
It can be removed. Please ignore.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r388075377
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
I am totally down to update the web console to reflect the new functionality.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r393598205
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/DefaultWorkerBehaviorConfig.java
##########
@@ -23,29 +23,52 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
import org.apache.druid.indexing.overlord.autoscaling.NoopAutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
*/
public class DefaultWorkerBehaviorConfig implements WorkerBehaviorConfig
{
- private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler();
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+ private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler(DEFAULT_AUTOSCALER_CATEGORY);
public static DefaultWorkerBehaviorConfig defaultConfig()
{
- return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER);
+ return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER, null);
}
private final WorkerSelectStrategy selectStrategy;
- private final AutoScaler autoScaler;
+ private final List<AutoScaler> autoScalers;
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
- @JsonProperty("autoScaler") AutoScaler autoScaler
+ @Deprecated @JsonProperty("autoScaler") AutoScaler autoScaler,
+ @JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
- this.autoScaler = autoScaler;
+ if (autoScaler != null && autoScalers != null) {
+ throw new IllegalArgumentException("The autoScaler and autoScalers properties are mutually exclusive");
+ }
+ if (autoScaler == null && autoScalers == null) {
+ throw new IllegalArgumentException("Either autoScaler or autoScalers property must be provided");
+ }
Review comment:
I made this change but now I get a LGTM alert. It complains that the autoScalers variable can be null in line 64 which would lead to an NPE, but in my opinion LGTM is wrong.
Can you advise?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387243419
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedWorkerProvisioningStrategy.java
##########
@@ -0,0 +1,659 @@
+/*
+ * 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.autoscaling;
+
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.inject.Inject;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.indexing.overlord.ImmutableWorkerInfo;
+import org.apache.druid.indexing.overlord.WorkerTaskRunner;
+import org.apache.druid.indexing.overlord.config.WorkerTaskRunnerConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.WorkerCategorySpec;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectUtils;
+import org.apache.druid.indexing.worker.Worker;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.concurrent.ScheduledExecutors;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+import org.joda.time.DateTime;
+import org.joda.time.Duration;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.stream.Collectors;
+
+/**
+ * Autoscaler provisioning strategy based on {@link AutoScaler#getCategory()} field. It selects autoscaler based on
+ * a worker's category.
+ */
+@JsonTypeName("categorizedTaskBased")
+public class CategorizedWorkerProvisioningStrategy extends AbstractWorkerProvisioningStrategy
Review comment:
I tried to refactor the code based on the above review comments. The class CategorizedWorkerProvisioningStrategy has been removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387199436
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/DefaultWorkerBehaviorConfig.java
##########
@@ -23,29 +23,42 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
import org.apache.druid.indexing.overlord.autoscaling.NoopAutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
*/
public class DefaultWorkerBehaviorConfig implements WorkerBehaviorConfig
{
- private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler();
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+ private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler(DEFAULT_AUTOSCALER_CATEGORY);
public static DefaultWorkerBehaviorConfig defaultConfig()
{
- return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER);
+ return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER, null);
}
private final WorkerSelectStrategy selectStrategy;
- private final AutoScaler autoScaler;
+ private final List<AutoScaler> autoScalers;
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
- @JsonProperty("autoScaler") AutoScaler autoScaler
+ @JsonProperty("autoScaler") AutoScaler autoScaler,
+ @JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
- this.autoScaler = autoScaler;
+ this.autoScalers = (autoScaler != null) ? Collections.singletonList(autoScaler) : autoScalers;
+ if (this.autoScalers == null) {
+ throw new IllegalArgumentException("Either autoScaler or autoScalers property needs to be provided");
Review comment:
can we add two more checks ?
1. make sure both `autoScaler` and `autoScalers` aren't provided
2. if `autoScalers` is provided, make sure each autoScaler element has a different category.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387245453
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
I tried to refactor the code based on the above review comments.
The class CategorizedWorkerBehaviorConfig has been removed, its logic been merged into the DefaultworkerBehaviorConfig.
The constructor has been modified according to himanshug's post above.
One thing I noticed recently, is that the new web-console is not treating the behaviour config as a single json object but has separate text boxes for select strategy and autoscaler.
What shall we do about this?
<img width="604" alt="overlord-dynamic-config" src="https://user-images.githubusercontent.com/1635350/75812158-05aa2f80-5d8e-11ea-90a5-3a5c64947078.png">
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387251212
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
That means, users who want to use `autoScalers` can't use the console UI to set the spec and would instead have to manually send the http request to coordinator using curl or whatever.
after this PR is merged, an issue should be created for console UI updates to make it possible to add `autoScalers` .
In this PR, we need to make sure that we stay totally backwards compatible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r391181268
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -184,54 +285,107 @@ public String apply(ImmutableWorkerInfo input)
@Override
public synchronized boolean doTerminate()
{
+ Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "terminate", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "terminate"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip terminating.");
return false;
}
boolean didTerminate = false;
+
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> pendingTasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ Set<String> allCategories = workersByCategories.keySet();
+ log.debug(
+ "Workers of %d categories: %s",
+ workersByCategories.size(),
+ allCategories
+ );
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doTerminate for workers of category %s", category);
+ continue;
+ }
+
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
Review comment:
nit: same here regarding HashSet instantiation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387813102
##########
File path: docs/configuration/index.md
##########
@@ -990,6 +985,22 @@ To view last <n> entries of the audit history of worker config issue a GET reque
http://<OVERLORD_IP>:<port>/druid/indexer/v1/worker/history?count=<n>
```
+##### Default Worker Config
+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`selectStrategy`|How to assign tasks to MiddleManagers. Choices are `fillCapacity`, `equalDistribution`, and `javascript`.|equalDistribution|
+|`autoScaler`|Only used if autoscaling is enabled. See below.|null|
+
+##### Categorized Worker Config
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387195374
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/ProvisioningUtil.java
##########
@@ -61,4 +78,96 @@ public boolean apply(ImmutableWorkerInfo worker)
};
}
+ @Nullable
+ public static DefaultWorkerBehaviorConfig getDefaultWorkerBehaviorConfig(
+ Supplier<WorkerBehaviorConfig> workerConfigRef,
+ String action
+ )
+ {
+ final WorkerBehaviorConfig workerBehaviorConfig = workerConfigRef.get();
+ if (workerBehaviorConfig == null) {
+ log.error("No workerConfig available, cannot %s workers.", action);
+ return null;
+ }
+ if (!(workerBehaviorConfig instanceof DefaultWorkerBehaviorConfig)) {
+ log.error(
+ "Only DefaultWorkerBehaviorConfig is supported as WorkerBehaviorConfig, [%s] given, cannot %s workers",
+ workerBehaviorConfig,
+ action
+ );
+ return null;
+ }
+ final DefaultWorkerBehaviorConfig workerConfig = (DefaultWorkerBehaviorConfig) workerBehaviorConfig;
+ if (workerConfig.getAutoScalers() == null) {
+ log.error("No autoScaler available, cannot %s workers", action);
+ return null;
+ }
+ return workerConfig;
+ }
+
+ @Nullable
+ public static WorkerCategorySpec getWorkerCategorySpec(DefaultWorkerBehaviorConfig workerConfig)
+ {
+ if (workerConfig != null && workerConfig.getSelectStrategy() != null) {
+ WorkerSelectStrategy selectStrategy = workerConfig.getSelectStrategy();
+ if (selectStrategy instanceof CategorizedWorkerSelectStrategy) {
+ return ((CategorizedWorkerSelectStrategy) selectStrategy).getWorkerCategorySpec();
+ }
+ }
+ return null;
+ }
+
+ public static Map<String, AutoScaler> mapAutoscalerByCategory(List<AutoScaler> autoScalers)
+ {
+ Map<String, AutoScaler> result = autoScalers.stream().collect(Collectors.groupingBy(
+ ProvisioningUtil::getAutoscalerCategory,
+ Collectors.collectingAndThen(Collectors.toList(), values -> values.get(0))
+ ));
+
+ if (result.size() != autoScalers.size()) {
+ log.warn(
Review comment:
Can we add the check in `DefaultWorkerBehaviorConfig` to ensure autoscalers with duplicate category aren't provided and fail? This is probably a case of user making an error in configuration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387864052
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/PendingTaskBasedWorkerProvisioningStrategy.java
##########
@@ -408,18 +521,50 @@ public String apply(Worker zkWorker)
return didTerminate;
}
+ private boolean initAutoscalers(DefaultWorkerBehaviorConfig workerConfig)
+ {
+ boolean didProvision = false;
+ for (AutoScaler autoScaler : workerConfig.getAutoScalers()) {
+ String category = ProvisioningUtil.getAutoscalerCategory(autoScaler);
+ didProvision = initAutoscaler(autoScaler, category, workerConfig, currentlyProvisioningMap) || didProvision;
+ }
+ return didProvision;
+ }
+
+ private boolean initAutoscaler(
+ AutoScaler autoScaler,
+ String category,
+ DefaultWorkerBehaviorConfig workerConfig,
+ Map<String, Set<String>> currentlyProvisioningMap
+ )
+ {
+ currentlyProvisioningMap.putIfAbsent(
+ category,
+ new HashSet<>()
+ );
+ Set<String> currentlyProvisioning = currentlyProvisioningMap.get(category);
Review comment:
nit: following replacement avoids instantiation of a new HashSet object on each call to this method.
```suggestion
Set<String> currentlyProvisioning = currentlyProvisioningMap.computeIfAbsent(
category,
ignored -> new HashSet<>()
);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r382714533
##########
File path: docs/configuration/index.md
##########
@@ -990,6 +985,22 @@ To view last <n> entries of the audit history of worker config issue a GET reque
http://<OVERLORD_IP>:<port>/druid/indexer/v1/worker/history?count=<n>
```
+##### Default Worker Config
+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`selectStrategy`|How to assign tasks to MiddleManagers. Choices are `fillCapacity`, `equalDistribution`, and `javascript`.|equalDistribution|
+|`autoScaler`|Only used if autoscaling is enabled. See below.|null|
+
+##### Categorized Worker Config
+Gives ability for overlord to work with several autoscaler setups to run tasks of different categories on clusters with different configurations.
+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`type`|Type of the config|required; must be `categorized`|
+|`selectStrategy`|How to assign tasks to MiddleManagers. Choices are `fillCapacity`, `equalDistribution`, and `javascript`.|equalDistribution|
+|`autoScalers`|List of [Autoscaler](#autoscaler) to serve tasks of appropriate category. In the list can be one autoscaler of default category (category declaration is omit). When [Worker Category Spec](#workercategoryspec) is not in strong assignment mode the default autoscaler will be used to serve tasks with categories which not have corresponding autoscaler|required; At least one autoscaler should be declared|
Review comment:
```suggestion
|`autoScalers`|List of [Autoscaler](#autoscaler) to serve tasks of appropriate category. In the list, there can be one autoscaler of default category (category declaration is omitted). When [Worker Category Spec](#workercategoryspec) is not in strong assignment mode then default autoscaler will be used to serve tasks with categories which do not have corresponding autoscaler|required; At least one autoscaler should be declared|
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r382716878
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedProvisioningConfig.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.autoscaling;
+
+import org.joda.time.Period;
+
+public class CategorizedProvisioningConfig extends PendingTaskBasedWorkerProvisioningConfig
Review comment:
note-to-self: Couldn't understand the point of this class as this is overriding all methods and just calling super.xx(..) , maybe I will understand more as I read rest of the PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] VladimirIordanov commented on a change in pull request
#9350: Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
VladimirIordanov commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r383159425
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedWorkerProvisioningStrategy.java
##########
@@ -0,0 +1,659 @@
+/*
+ * 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.autoscaling;
+
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.inject.Inject;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.indexing.overlord.ImmutableWorkerInfo;
+import org.apache.druid.indexing.overlord.WorkerTaskRunner;
+import org.apache.druid.indexing.overlord.config.WorkerTaskRunnerConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.WorkerCategorySpec;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectUtils;
+import org.apache.druid.indexing.worker.Worker;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.concurrent.ScheduledExecutors;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+import org.joda.time.DateTime;
+import org.joda.time.Duration;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.stream.Collectors;
+
+/**
+ * Autoscaler provisioning strategy based on {@link AutoScaler#getCategory()} field. It selects autoscaler based on
+ * a worker's category.
+ */
+@JsonTypeName("categorizedTaskBased")
+public class CategorizedWorkerProvisioningStrategy extends AbstractWorkerProvisioningStrategy
Review comment:
After reviewing the difference I recalled that it is leftover which can be removed along with CategorizedProvisioningConfig. Originally I separately created the new strategy as an intermediate step to check that I didn't affect the original strategy. So I just forgot to remove it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387813266
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/CategorizedWorkerBehaviorConfig.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.setup;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
+ */
+public class CategorizedWorkerBehaviorConfig implements WorkerBehaviorConfig
+{
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+
+ private final WorkerSelectStrategy selectStrategy;
+ private final List<AutoScaler> autoScalers;
+
+ @JsonCreator
+ public CategorizedWorkerBehaviorConfig(
Review comment:
got it. thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r385098914
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedProvisioningConfig.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.autoscaling;
+
+import org.joda.time.Period;
+
+public class CategorizedProvisioningConfig extends PendingTaskBasedWorkerProvisioningConfig
Review comment:
Thanks Vladi.
Just to update the status: I removed all orhpaned classes.
Changes to the DefaultWorkerBehaviorConfig class are still pending
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r382888794
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/CategorizedWorkerProvisioningStrategy.java
##########
@@ -0,0 +1,659 @@
+/*
+ * 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.autoscaling;
+
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
+import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.inject.Inject;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.indexing.overlord.ImmutableWorkerInfo;
+import org.apache.druid.indexing.overlord.WorkerTaskRunner;
+import org.apache.druid.indexing.overlord.config.WorkerTaskRunnerConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.CategorizedWorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerBehaviorConfig;
+import org.apache.druid.indexing.overlord.setup.WorkerCategorySpec;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectStrategy;
+import org.apache.druid.indexing.overlord.setup.WorkerSelectUtils;
+import org.apache.druid.indexing.worker.Worker;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.concurrent.ScheduledExecutors;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+import org.joda.time.DateTime;
+import org.joda.time.Duration;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.stream.Collectors;
+
+/**
+ * Autoscaler provisioning strategy based on {@link AutoScaler#getCategory()} field. It selects autoscaler based on
+ * a worker's category.
+ */
+@JsonTypeName("categorizedTaskBased")
+public class CategorizedWorkerProvisioningStrategy extends AbstractWorkerProvisioningStrategy
Review comment:
Can you explain how this is different from `PendingTaskBasedWorkerProvisioningStrategy` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r391175284
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/PendingTaskBasedWorkerProvisioningStrategy.java
##########
@@ -157,24 +125,94 @@ public synchronized boolean doProvision()
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig = getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<Task>> tasksByCategories = pendingTasks.stream().collect(Collectors.groupingBy(
+ task -> WorkerSelectUtils.getTaskCategory(
+ task,
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ )
+ ));
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<Task> categoryTasks = tasksByCategories.getOrDefault(category, Collections.emptyList());
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
Review comment:
nit: following replacement avoids instantiation of a new HashSet object on each call to this method.
```suggestion
Set<String> currentlyProvisioning = currentlyProvisioningMap.computeIfAbsent(
category,
ignored -> new HashSet<>()
);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387812839
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/ProvisioningUtil.java
##########
@@ -61,4 +78,96 @@ public boolean apply(ImmutableWorkerInfo worker)
};
}
+ @Nullable
+ public static DefaultWorkerBehaviorConfig getDefaultWorkerBehaviorConfig(
+ Supplier<WorkerBehaviorConfig> workerConfigRef,
+ String action
+ )
+ {
+ final WorkerBehaviorConfig workerBehaviorConfig = workerConfigRef.get();
+ if (workerBehaviorConfig == null) {
+ log.error("No workerConfig available, cannot %s workers.", action);
+ return null;
+ }
+ if (!(workerBehaviorConfig instanceof DefaultWorkerBehaviorConfig)) {
+ log.error(
+ "Only DefaultWorkerBehaviorConfig is supported as WorkerBehaviorConfig, [%s] given, cannot %s workers",
+ workerBehaviorConfig,
+ action
+ );
+ return null;
+ }
+ final DefaultWorkerBehaviorConfig workerConfig = (DefaultWorkerBehaviorConfig) workerBehaviorConfig;
+ if (workerConfig.getAutoScalers() == null) {
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r395965110
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
+
+ didProvision = doProvision(
Review comment:
I see, I spent a little more time today to read about the effects of resolving in https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews#resolving-conversations . Resolving basically folds the conversation so as reviewer, I can see, you might not want that to happen till you verify whether your comment is addressed.
However, In that case reviewers also should have the responsibility of actively marking their comments resolved as and when they are addressed or else PR author is in doubt whether the change has been accepted by reviewer. I haven't really been doing that while reviewing the PRs and that is probably what confused @sascha-coenen .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387831262
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/DefaultWorkerBehaviorConfig.java
##########
@@ -23,29 +23,52 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
import org.apache.druid.indexing.overlord.autoscaling.NoopAutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
*/
public class DefaultWorkerBehaviorConfig implements WorkerBehaviorConfig
{
- private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler();
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+ private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler(DEFAULT_AUTOSCALER_CATEGORY);
public static DefaultWorkerBehaviorConfig defaultConfig()
{
- return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER);
+ return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER, null);
}
private final WorkerSelectStrategy selectStrategy;
- private final AutoScaler autoScaler;
+ private final List<AutoScaler> autoScalers;
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
- @JsonProperty("autoScaler") AutoScaler autoScaler
+ @Deprecated @JsonProperty("autoScaler") AutoScaler autoScaler,
+ @JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
- this.autoScaler = autoScaler;
+ if (autoScaler != null && autoScalers != null) {
+ throw new IllegalArgumentException("The autoScaler and autoScalers properties are mutually exclusive");
+ }
+ if (autoScaler == null && autoScalers == null) {
+ throw new IllegalArgumentException("Either autoScaler or autoScalers property must be provided");
+ }
Review comment:
nit: XOR operator is the usual way to handle this type of check.
```suggestion
if (autoScaler == null ^ autoScalers == null) {
throw new IllegalArgumentException("Either(and only one of) autoScaler or autoScalers property must be provided");
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r393606761
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -184,54 +285,107 @@ public String apply(ImmutableWorkerInfo input)
@Override
public synchronized boolean doTerminate()
{
+ Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "terminate", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "terminate"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip terminating.");
return false;
}
boolean didTerminate = false;
+
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> pendingTasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ Set<String> allCategories = workersByCategories.keySet();
+ log.debug(
+ "Workers of %d categories: %s",
+ workersByCategories.size(),
+ allCategories
+ );
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doTerminate for workers of category %s", category);
+ continue;
+ }
+
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r387185398
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/setup/DefaultWorkerBehaviorConfig.java
##########
@@ -23,29 +23,42 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.indexing.overlord.autoscaling.AutoScaler;
import org.apache.druid.indexing.overlord.autoscaling.NoopAutoScaler;
+import org.apache.druid.indexing.worker.config.WorkerConfig;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
/**
+ * This configuration allows overlord to work with several autoscalers to run tasks of different categories.
*/
public class DefaultWorkerBehaviorConfig implements WorkerBehaviorConfig
{
- private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler();
+ // Use the same category constant as for worker category to match default workers and autoscalers
+ public static final String DEFAULT_AUTOSCALER_CATEGORY = WorkerConfig.DEFAULT_CATEGORY;
+ private static final AutoScaler DEFAULT_AUTOSCALER = new NoopAutoScaler(DEFAULT_AUTOSCALER_CATEGORY);
public static DefaultWorkerBehaviorConfig defaultConfig()
{
- return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER);
+ return new DefaultWorkerBehaviorConfig(DEFAULT_STRATEGY, DEFAULT_AUTOSCALER, null);
}
private final WorkerSelectStrategy selectStrategy;
- private final AutoScaler autoScaler;
+ private final List<AutoScaler> autoScalers;
@JsonCreator
public DefaultWorkerBehaviorConfig(
@JsonProperty("selectStrategy") WorkerSelectStrategy selectStrategy,
- @JsonProperty("autoScaler") AutoScaler autoScaler
+ @JsonProperty("autoScaler") AutoScaler autoScaler,
+ @JsonProperty("autoScalers") List<AutoScaler> autoScalers
)
{
this.selectStrategy = selectStrategy;
- this.autoScaler = autoScaler;
+ this.autoScalers = (autoScaler != null) ? Collections.singletonList(autoScaler) : autoScalers;
+ if (this.autoScalers == null) {
+ throw new IllegalArgumentException("Either autoScaler or autoScalers property needs to be provided");
Review comment:
can we remove mention of `autoScaler` property and deprecate it .. so that it gets removed eventually.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on issue #9350: Overlord to support
autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on issue #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#issuecomment-601956690
+1 after https://github.com/apache/druid/pull/9350#discussion_r395933753 is addressed. thanks for your patience.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r393606276
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
+
+ didProvision = doProvision(
Review comment:
you are perfectly right about the repeated code logic. However, I'm not able to refactor anything. For one thing, the way that responsibilities are spread across classes seems to make it impossible to refactor just the affected methods but would require a larger rewrite.
On a pragmatic level I'm also facing the issue that executing "mvn test" on just the druid-indexing module takes 15 minutes. If I try to execute the respective testsuite alone (SimpleProvisioningStrategyTest) in IntelliJ, I get an ANTL error:
```
/opt/repos/druid/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java
Error:(28, 40) java: package org.apache.druid.math.expr.antlr does not exist
```
Can you advise?
Also, one innocent question: I see these "resolve conversation" buttons and wonder whether I am supposed to mark a conversation as resolved if I believe to have addressed a review comment or whether it is rather meant to be the reviewer's privilege to do that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r393606809
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] sascha-coenen commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
sascha-coenen commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r393607190
##########
File path: services/src/main/java/org/apache/druid/cli/CliOverlord.java
##########
@@ -353,6 +354,7 @@ private void configureOverlordHelpers(Binder binder)
}
/**
+ *
Review comment:
;)
right. I reverted the file.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r391175967
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/PendingTaskBasedWorkerProvisioningStrategy.java
##########
@@ -333,67 +377,136 @@ public synchronized boolean doTerminate()
{
Collection<ImmutableWorkerInfo> zkWorkers = runner.getWorkers();
log.debug("Workers: %d [%s]", zkWorkers.size(), zkWorkers);
- final DefaultWorkerBehaviorConfig workerConfig = getDefaultWorkerBehaviorConfig(workerConfigRef, "terminate", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "terminate"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip terminating.");
return false;
}
- log.info("Currently provisioning: %d %s", currentlyProvisioning.size(), currentlyProvisioning);
- if (!currentlyProvisioning.isEmpty()) {
- log.debug("Already provisioning nodes, Not Terminating any nodes.");
- return false;
+ boolean didTerminate = false;
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(zkWorkers);
+
+ Set<String> allCategories = workersByCategories.keySet();
+ log.debug(
+ "Workers of %d categories: %s",
+ workersByCategories.size(),
+ allCategories
+ );
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.getOrDefault(
+ category,
+ Collections.emptySet()
+ );
+ log.info(
+ "Currently provisioning of category %s: %d %s",
+ category,
+ currentlyProvisioning.size(),
+ currentlyProvisioning
+ );
+ if (!currentlyProvisioning.isEmpty()) {
+ log.debug("Already provisioning nodes of category %s, Not Terminating any nodes.", category);
+ return false;
+ }
+
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doTerminate for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
Review comment:
nit: following replacement avoids instantiation of a new HashSet object on each call to this method.
```suggestion
Set<String> currentlyTerminating = currentlyTerminatingMap.computeIfAbsent(
category,
ignored -> new HashSet<>()
);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on issue #9350: Overlord to support
autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#issuecomment-597923651
This pull request **introduces 1 alert** when merging cd3c76dfd73d6d3d9532a94c6cb5748d4f41e763 into 2ef5c17441a450171523653787743db76a3e3bdb - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-bddb60c8b5560f03e1f129ccb4521353368b64be)
**new alerts:**
* 1 for Dereferenced variable may be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
himanshug commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r391181996
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
+
+ didProvision = doProvision(
Review comment:
nit: code till here appears to be repeated in many places, if possible, then refactor it into ProvisioningUtil maybe
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9350:
Overlord to support autoscalers per indexer/middlemanager category
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9350: Overlord to support autoscalers per indexer/middlemanager category
URL: https://github.com/apache/druid/pull/9350#discussion_r395940463
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/autoscaling/SimpleWorkerProvisioningStrategy.java
##########
@@ -102,69 +102,170 @@ public Provisioner makeProvisioner(WorkerTaskRunner runner)
private final WorkerTaskRunner runner;
private final ScalingStats scalingStats = new ScalingStats(config.getNumEventsToTrack());
- private final Set<String> currentlyProvisioning = new HashSet<>();
- private final Set<String> currentlyTerminating = new HashSet<>();
+ private final Map<String, Set<String>> currentlyProvisioningMap = new HashMap<>();
+ private final Map<String, Set<String>> currentlyTerminatingMap = new HashMap<>();
- private int targetWorkerCount = -1;
- private DateTime lastProvisionTime = DateTimes.nowUtc();
- private DateTime lastTerminateTime = lastProvisionTime;
+ private final Map<String, Integer> targetWorkerCountMap = new HashMap<>();
+ private final Map<String, DateTime> lastProvisionTimeMap = new HashMap<>();
+ private final Map<String, DateTime> lastTerminateTimeMap = new HashMap<>();
SimpleProvisioner(WorkerTaskRunner runner)
{
this.runner = runner;
}
+ private Map<String, List<TaskRunnerWorkItem>> groupTasksByCategories(
+ Collection<? extends TaskRunnerWorkItem> pendingTasks,
+ WorkerTaskRunner runner,
+ WorkerCategorySpec workerCategorySpec
+ )
+ {
+ Collection<Task> pendingTasksPayload = runner.getPendingTaskPayloads();
+ Map<String, List<Task>> taskPayloadsById = pendingTasksPayload.stream()
+ .collect(Collectors.groupingBy(Task::getId));
+
+ return pendingTasks.stream().collect(Collectors.groupingBy(task -> {
+ List<Task> taskPayloads = taskPayloadsById.get(task.getTaskId());
+ if (taskPayloads == null || taskPayloads.isEmpty()) {
+ return DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY;
+ }
+ return WorkerSelectUtils.getTaskCategory(
+ taskPayloads.get(0),
+ workerCategorySpec,
+ DefaultWorkerBehaviorConfig.DEFAULT_AUTOSCALER_CATEGORY
+ );
+ }));
+ }
+
@Override
public synchronized boolean doProvision()
{
Collection<? extends TaskRunnerWorkItem> pendingTasks = runner.getPendingTasks();
+ log.debug("Pending tasks: %d %s", pendingTasks.size(), pendingTasks);
Collection<ImmutableWorkerInfo> workers = runner.getWorkers();
+ log.debug("Workers: %d %s", workers.size(), workers);
boolean didProvision = false;
- final DefaultWorkerBehaviorConfig workerConfig =
- PendingTaskBasedWorkerProvisioningStrategy.getDefaultWorkerBehaviorConfig(workerConfigRef, "provision", log);
+ final DefaultWorkerBehaviorConfig workerConfig = ProvisioningUtil.getDefaultWorkerBehaviorConfig(
+ workerConfigRef,
+ "provision"
+ );
if (workerConfig == null) {
+ log.info("No worker config found. Skip provisioning.");
return false;
}
+ WorkerCategorySpec workerCategorySpec = ProvisioningUtil.getWorkerCategorySpec(workerConfig);
+
+ // Group tasks by categories
+ Map<String, List<TaskRunnerWorkItem>> tasksByCategories = groupTasksByCategories(
+ pendingTasks,
+ runner,
+ workerCategorySpec
+ );
+
+ Map<String, List<ImmutableWorkerInfo>> workersByCategories = ProvisioningUtil.getWorkersByCategories(workers);
+
+ // Merge categories of tasks and workers
+ Set<String> allCategories = new HashSet<>(tasksByCategories.keySet());
+ allCategories.addAll(workersByCategories.keySet());
+
+ log.debug(
+ "Pending Tasks of %d categories (%s), Workers of %d categories (%s). %d common categories: %s",
+ tasksByCategories.size(),
+ tasksByCategories.keySet(),
+ workersByCategories.size(),
+ workersByCategories.keySet(),
+ allCategories.size(),
+ allCategories
+ );
+
+ if (allCategories.isEmpty()) {
+ // Likely empty categories means initialization.
+ // Just try to spinup required amount of workers of each non empty autoscalers
+ return initAutoscalers(workerConfig);
+ }
+
+ Map<String, AutoScaler> autoscalersByCategory = ProvisioningUtil.mapAutoscalerByCategory(workerConfig.getAutoScalers());
+
+ for (String category : allCategories) {
+ List<? extends TaskRunnerWorkItem> categoryTasks = tasksByCategories.getOrDefault(
+ category,
+ Collections.emptyList()
+ );
+ AutoScaler categoryAutoscaler = ProvisioningUtil.getAutoscalerByCategory(category, autoscalersByCategory);
+
+ if (categoryAutoscaler == null) {
+ log.error("No autoScaler available, cannot execute doProvision for workers of category %s", category);
+ continue;
+ }
+ // Correct category name by selected autoscaler
+ category = ProvisioningUtil.getAutoscalerCategory(categoryAutoscaler);
+
+ List<ImmutableWorkerInfo> categoryWorkers = workersByCategories.getOrDefault(category, Collections.emptyList());
+ currentlyProvisioningMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyProvisioning = this.currentlyProvisioningMap.get(category);
+ currentlyTerminatingMap.putIfAbsent(category, new HashSet<>());
+ Set<String> currentlyTerminating = this.currentlyTerminatingMap.get(category);
+
+ didProvision = doProvision(
Review comment:
Just to randomly chime in, as a reviewer I definitely prefer that the author _does not_ mark my comments as resolved, it makes it harder to come back to the PR and find all of my comments and check to make sure that they are indeed resolved. I think it would be better for whoever left the review comment to resolve it when they are satisfied, but maybe we should reach a consensus as a community on this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org