jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r705935477
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java
##########
@@ -650,36 +653,38 @@ String getMaskedCommand(List<String> maskedProperties,
List<String> command)
}
@Override
- public long getTotalTaskSlotCount()
+ public Map<String, Long> getTotalTaskSlotCount()
{
if (config.getPorts() != null && !config.getPorts().isEmpty()) {
- return config.getPorts().size();
+ return ImmutableMap.of(workerConfig.getCategory(),
Long.valueOf(config.getPorts().size()));
}
- return config.getEndPort() - config.getStartPort() + 1;
+ return ImmutableMap.of(workerConfig.getCategory(),
Long.valueOf(config.getEndPort() - config.getStartPort() + 1));
}
@Override
- public long getIdleTaskSlotCount()
+ public Map<String, Long> getIdleTaskSlotCount()
{
- return Math.max(getTotalTaskSlotCount() - getUsedTaskSlotCount(), 0);
+ Map<String, Long> totalTaskSlots = getTotalTaskSlotCount();
+ Map<String, Long> usedTaskSlots = getUsedTaskSlotCount();
+ return ImmutableMap.of(workerConfig.getCategory(),
Math.max(totalTaskSlots.get(workerConfig.getCategory()) -
usedTaskSlots.get(workerConfig.getCategory()), 0));
Review comment:
nit: since there is always only one entry in the map in the
middleManager, it would seem clearer if you add another methods returning long
values of `totalTaskSlotCount` and `idleTaskSlotCount` and use them here
instead of getting them from the map.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ThreadingTaskRunner.java
##########
@@ -454,33 +456,35 @@ public RunnerTaskState getRunnerTaskState(String taskId)
}
@Override
- public long getTotalTaskSlotCount()
+ public Map<String, Long> getTotalTaskSlotCount()
{
- return workerConfig.getCapacity();
+ return ImmutableMap.of(workerConfig.getCategory(),
Long.valueOf(workerConfig.getCapacity()));
}
@Override
- public long getIdleTaskSlotCount()
+ public Map<String, Long> getIdleTaskSlotCount()
{
- return Math.max(getTotalTaskSlotCount() - getUsedTaskSlotCount(), 0);
+ Map<String, Long> totalTaskSlots = getTotalTaskSlotCount();
+ Map<String, Long> usedTaskSlots = getTotalTaskSlotCount();
+ return ImmutableMap.of(workerConfig.getCategory(),
Math.max(totalTaskSlots.get(workerConfig.getCategory()) -
usedTaskSlots.get(workerConfig.getCategory()), 0));
Review comment:
nit: same here. Since there is always only one entry in the map in the
indexer, it would seem clearer if you add another methods returning long values
of totalTaskSlotCount and idleTaskSlotCount and use them here instead of
getting them from the map.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/SingleTaskBackgroundRunner.java
##########
@@ -314,33 +317,33 @@ public TaskLocation getTaskLocation(String taskId)
}
@Override
- public long getTotalTaskSlotCount()
+ public Map<String, Long> getTotalTaskSlotCount()
{
- return 1;
+ return ImmutableMap.of(WorkerConfig.DEFAULT_CATEGORY, 1L);
Review comment:
AFAIT, the returned value is currently not used in anywhere. @mghosh4 is
this correct? If so, how about throwing `UnsupportedOperationException` instead
to make it more obvious?
##########
File path: website/.spelling
##########
@@ -1102,6 +1102,7 @@ P3M
PT12H
STRING_ARRAY
String.format
+UNNEST
Review comment:
Is this change intentional? What is this change for? If not, please
revert unnecessary changes.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java
##########
@@ -1568,55 +1569,65 @@ void taskAddedOrUpdated(final TaskAnnouncement
announcement, final WorkerHolder
}
@Override
- public long getTotalTaskSlotCount()
+ public Map<String, Long> getTotalTaskSlotCount()
{
- long totalPeons = 0;
+ Map<String, Long> totalPeons = new HashMap<>();
for (ImmutableWorkerInfo worker : getWorkers()) {
- totalPeons += worker.getWorker().getCapacity();
+ String workerCategory = worker.getWorker().getCategory();
+ int workerCapacity = worker.getWorker().getCapacity();
+ totalPeons.put(workerCategory, totalPeons.getOrDefault(workerCategory,
0L) + workerCapacity);
Review comment:
nit: same suggestion to use `compute` instead.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java
##########
@@ -1439,55 +1440,65 @@ RemoteTaskRunnerConfig getRemoteTaskRunnerConfig()
}
@Override
- public long getTotalTaskSlotCount()
+ public Map<String, Long> getTotalTaskSlotCount()
{
- long totalPeons = 0;
+ Map<String, Long> totalPeons = new HashMap<>();
for (ImmutableWorkerInfo worker : getWorkers()) {
- totalPeons += worker.getWorker().getCapacity();
+ String workerCategory = worker.getWorker().getCategory();
+ int workerCapacity = worker.getWorker().getCapacity();
+ totalPeons.put(workerCategory, totalPeons.getOrDefault(workerCategory,
0L) + workerCapacity);
Review comment:
nit:
```suggestion
totalPeons.compute(
workerCategory,
(category, totalCapacity) -> totalCapacity == null ?
workerCapacity : totalCapacity + workerCapacity
);
```
since this will compute the hash only once. Same comment for other places
where the same pattern applies.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
##########
@@ -124,14 +125,14 @@ default TaskLocation getTaskLocation(String taskId)
/**
* APIs useful for emitting statistics for @TaskSlotCountStatsMonitor
- */
- long getTotalTaskSlotCount();
+ */
+ Map<String, Long> getTotalTaskSlotCount();
Review comment:
nit: could use `Object2LongMap`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
