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 2021/08/05 21:45:43 UTC
[GitHub] [druid] nikhil-ddu opened a new pull request #11554: Add worker category dimension
nikhil-ddu opened a new pull request #11554:
URL: https://github.com/apache/druid/pull/11554
Fixes #11505.
### Description
#### Changed APIs of `TaskRunner`
Changed following APIs of `TaskRunner` used for for emitting statistics for @TaskSlotCountStatsMonitor to return `Map<String, Long>` where key will be worker category and value will be particular metric value.
```
long getTotalTaskSlotCount();
long getIdleTaskSlotCount();
long getUsedTaskSlotCount();
long getLazyTaskSlotCount();
long getBlacklistedTaskSlotCount();
```
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] 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/druid/blob/master/dev/license.md)
- [ ] 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, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #11554:
URL: https://github.com/apache/druid/pull/11554
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r706418639
##########
File path: website/.spelling
##########
@@ -1102,6 +1102,7 @@ P3M
PT12H
STRING_ARRAY
String.format
+UNNEST
Review comment:
Hmm, this is strange because that word should have been added in another PR. Maybe it was because some necessary change was not in master yet when the CI ran last time. Can we revert this change and see what Travis says now?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r707906935
##########
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:
I would agree that returning 1 is reasonable if there was no category returned in this method as it was before. Now, this method always returns the default category, which could be wrong when your middleManager has a different category configured. It seems a bit clearer to me if it throws `UnsupportedOperationException`. In any case, it would be great if you can add some comment saying "This method should be never called in peons".
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r705974407
##########
File path: website/.spelling
##########
@@ -1102,6 +1102,7 @@ P3M
PT12H
STRING_ARRAY
String.format
+UNNEST
Review comment:
had to add it because some checks were failing because of it https://app.travis-ci.com/github/apache/druid/jobs/530470206
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r705973673
##########
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:
since I can't change return value of `getTotalTaskSlotCount()` method, are you saying that I shall add another method like `public long getTotalTaskSlotCountLong()`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r706535993
##########
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:
If it's okay I'm not planning to address this change in this 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r752902529
##########
File path: extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
##########
@@ -63,11 +63,11 @@
"task/pending/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
"task/waiting/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
- "taskSlot/total/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/idle/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/busy/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/lazy/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/blacklisted/count" : { "dimensions" : [], "type" : "gauge" },
+ "taskSlot/total/count" : { "dimensions" : ["category"], "type" : "gauge" },
Review comment:
This is worth discussing, but I think we can do it in #11958.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #11554:
URL: https://github.com/apache/druid/pull/11554#issuecomment-973802867
I'm merging this PR. Thanks @nikhil-ddu.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
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 #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r736187832
##########
File path: extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
##########
@@ -63,11 +63,11 @@
"task/pending/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
"task/waiting/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
- "taskSlot/total/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/idle/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/busy/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/lazy/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/blacklisted/count" : { "dimensions" : [], "type" : "gauge" },
+ "taskSlot/total/count" : { "dimensions" : ["category"], "type" : "gauge" },
Review comment:
hmm, it looks like other emitters have files like this too:
* prometheus-emitter: https://github.com/apache/druid/blob/master/extensions-contrib/prometheus-emitter/src/main/resources/defaultMetrics.json
* opentsdb-emitter: https://github.com/apache/druid/blob/master/extensions-contrib/opentsdb-emitter/src/main/resources/defaultMetrics.json
* dropwizard-emitter: https://github.com/apache/druid/blob/master/extensions-contrib/dropwizard-emitter/src/main/resources/defaultMetricDimensions.json
* graphite-emitter: https://github.com/apache/druid/blob/master/extensions-contrib/graphite-emitter/src/main/resources/defaultWhiteListMap.json
* ambari-emitter: https://github.com/apache/druid/blob/master/extensions-contrib/ambari-metrics-emitter/src/main/resources/defaultWhiteListMap.json
should they be updated as well after this change? I didn't look _super_ closely at their contents, and i'm not sure how consistent they actually are with each other since I'm not sure they are currently emitting these metrics at all... but this seemed worth discussing.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r724727761
##########
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:
Hi @nikhil-ddu, can you please check my last comment above? The CI failure seems legit as well. It failed with a compilation error like below. I think this PR is good to go once you fix the compilation error.
```
[ERROR] COMPILATION ERROR :
[ERROR] /home/travis/build/apache/druid/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/SinglePhaseSubTask.java:[622,13] constructor IngestionStatsAndErrorsTaskReportData in class org.apache.druid.indexing.common.IngestionStatsAndErrorsTaskReportData cannot be applied to given types;
required: org.apache.druid.indexer.IngestionState,java.util.Map<java.lang.String,java.lang.Object>,java.util.Map<java.lang.String,java.lang.Object>,java.lang.String,boolean,long
found: org.apache.druid.indexer.IngestionState,java.util.Map<java.lang.String,java.lang.Object>,java.util.Map<java.lang.String,java.lang.Object>,java.lang.String,boolean
reason: actual and formal argument lists differ in length
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project druid-indexing-service: Compilation failure
[ERROR] /home/travis/build/apache/druid/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/SinglePhaseSubTask.java:[622,13] constructor IngestionStatsAndErrorsTaskReportData in class org.apache.druid.indexing.common.IngestionStatsAndErrorsTaskReportData cannot be applied to given types;
[ERROR] required: org.apache.druid.indexer.IngestionState,java.util.Map<java.lang.String,java.lang.Object>,java.util.Map<java.lang.String,java.lang.Object>,java.lang.String,boolean,long
[ERROR] found: org.apache.druid.indexer.IngestionState,java.util.Map<java.lang.String,java.lang.Object>,java.util.Map<java.lang.String,java.lang.Object>,java.lang.String,boolean
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r752901925
##########
File path: extensions-contrib/statsd-emitter/src/main/resources/defaultMetricDimensions.json
##########
@@ -63,11 +63,11 @@
"task/pending/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
"task/waiting/count" : { "dimensions" : ["dataSource"], "type" : "gauge" },
- "taskSlot/total/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/idle/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/busy/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/lazy/count" : { "dimensions" : [], "type" : "gauge" },
- "taskSlot/blacklisted/count" : { "dimensions" : [], "type" : "gauge" },
+ "taskSlot/total/count" : { "dimensions" : ["category"], "type" : "gauge" },
Review comment:
I created https://github.com/apache/druid/issues/11958.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r706417740
##########
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:
Yes, that is what I meant.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on pull request #11554:
URL: https://github.com/apache/druid/pull/11554#issuecomment-948134643
@jihoonson I've updated the PR with changes you requested for. Can you merge 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r707784563
##########
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:
@jihoonson yes you are correct this value is not used anywhere except `SingleTaskBackgroundRunnerTest`. Returning 1 is probably the right thing to do here since in a sense there is only 1 task slot. Ofcourse throwing the exception is an option as well. I would personally tilt towards the first option given it fits the semantics better.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r706420606
##########
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:
I'm not sure what you mean. My suggestion is changing all return type here including all their implementations. For singleton map, you can use `Object2LongMaps.singleton()`. But this comment is nit because this api is not performance critical.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r705973673
##########
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:
since I can't change return value of `getTotalTaskSlotCount()` method because it's overridden, are you saying that I shall add another method like `public long getTotalTaskSlotCountLong()`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nikhil-ddu commented on a change in pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
nikhil-ddu commented on a change in pull request #11554:
URL: https://github.com/apache/druid/pull/11554#discussion_r705996709
##########
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:
Since I'm returning `ImmutableMap` from methods where there will be only one entry, I can't use `Object2LongMap` without changing return values.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
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 pull request #11554: Add worker category dimension
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11554:
URL: https://github.com/apache/druid/pull/11554#issuecomment-893871724
This pull request **introduces 10 alerts** when merging 235eefc6bd04655b2bd309f230100fe7e9d2fc1a into 257bc5c62fda5966e6c22b2d95be3fcf433be1c8 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-dbdb10f124ca76d56d2a9c84b49d13941a5e30a0)
**new alerts:**
* 10 for Boxed variable is never 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org