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 2022/01/03 08:30:10 UTC

[GitHub] [druid] isandeep41 opened a new pull request #12111: add a new query laning metrics to visualize lane assignment

isandeep41 opened a new pull request #12111:
URL: https://github.com/apache/druid/pull/12111


   This PR aims to introduce a new metrics `query/priority` which will help to visualize the assigned lane if the [Laning Strategy][1] is enabled.
   
   ![image-20220103155445805](https://user-images.githubusercontent.com/3384729/147911023-3b91bc05-be19-4300-97e3-e37c84cd0fd7.png)
   The Grafana Dashboard showing last hour `high/low` lanes distribution on a Druid cluster running Druid version 0.22.1
   
   [1]: https://druid.apache.org/docs/latest/configuration/index.html#laning-strategies "Laning strategies"
   
   
   
   <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.
      - [ ] 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.
   - [ ] 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 commented on pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #12111:
URL: https://github.com/apache/druid/pull/12111#issuecomment-1059614441


   Thank you @gianm!


-- 
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] gianm commented on pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #12111:
URL: https://github.com/apache/druid/pull/12111#issuecomment-1059613007


   I included a fix in #12307. It was a logical conflict between this patch and #12202, that as far as I can tell only affected the test case QueryableModuleTest.


-- 
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] isandeep41 commented on a change in pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
isandeep41 commented on a change in pull request #12111:
URL: https://github.com/apache/druid/pull/12111#discussion_r778234360



##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -108,6 +140,7 @@ public QueryScheduler(
       this.totalCapacity = serverConfig.getNumThreads();
     }
     this.laneRegistry = BulkheadRegistry.of(getLaneConfigs(limitTotal));
+    this.emitter = new ServiceEmitter("test", "localhost", new NoopEmitter());

Review comment:
       agree, have removed the ctor and the object is now directly initiated from the test classes




-- 
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] FrankChen021 commented on a change in pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12111:
URL: https://github.com/apache/druid/pull/12111#discussion_r777789692



##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -108,6 +140,7 @@ public QueryScheduler(
       this.totalCapacity = serverConfig.getNumThreads();
     }
     this.laneRegistry = BulkheadRegistry.of(getLaneConfigs(limitTotal));
+    this.emitter = new ServiceEmitter("test", "localhost", new NoopEmitter());

Review comment:
       So this ctor is for the test case only? I think we'd better not instantiate such object here but pass an object from the test cases.




-- 
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] isandeep41 closed pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
isandeep41 closed pull request #12111:
URL: https://github.com/apache/druid/pull/12111


   


-- 
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] isandeep41 commented on a change in pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
isandeep41 commented on a change in pull request #12111:
URL: https://github.com/apache/druid/pull/12111#discussion_r778234566



##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -86,7 +91,34 @@
    * but it is OK in most cases since they will be cleaned up once the query is done.
    */
   private final SetMultimap<String, String> queryDatasources;
+  private final ServiceEmitter emitter;
 
+  public QueryScheduler(
+      int totalNumThreads,
+      QueryPrioritizationStrategy prioritizationStrategy,
+      QueryLaningStrategy laningStrategy,
+      ServerConfig serverConfig,
+      ServiceEmitter emitter
+  )
+  {
+    this.prioritizationStrategy = prioritizationStrategy;
+    this.laningStrategy = laningStrategy;
+    this.queryFutures = Multimaps.synchronizedSetMultimap(HashMultimap.create());
+    this.queryDatasources = Multimaps.synchronizedSetMultimap(HashMultimap.create());
+    // if totalNumThreads is above 0 and less than druid.server.http.numThreads, enforce total limit
+    final boolean limitTotal;
+    if (totalNumThreads > 0 && totalNumThreads < serverConfig.getNumThreads()) {
+      limitTotal = true;
+      this.totalCapacity = totalNumThreads;
+    } else {
+      limitTotal = false;
+      this.totalCapacity = serverConfig.getNumThreads();
+    }
+    this.laneRegistry = BulkheadRegistry.of(getLaneConfigs(limitTotal));
+    this.emitter = emitter;
+  }
+
+  @VisibleForTesting

Review comment:
       the ctor 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.

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] FrankChen021 commented on a change in pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12111:
URL: https://github.com/apache/druid/pull/12111#discussion_r777788353



##########
File path: server/src/main/java/org/apache/druid/server/QuerySchedulerProvider.java
##########
@@ -23,27 +23,28 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import org.apache.druid.java.util.emitter.service.ServiceEmitter;
 import org.apache.druid.server.initialization.ServerConfig;
 
 
 public class QuerySchedulerProvider extends QuerySchedulerConfig implements Provider<QueryScheduler>
 {
   private final ServerConfig serverConfig;
-
+  private ServiceEmitter emitter;

Review comment:
       ```suggestion
     private final ServiceEmitter emitter;
   ```

##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -137,6 +170,12 @@ public void registerQueryFuture(Query<?> query, ListenableFuture<?> future)
     Optional<Integer> priority = prioritizationStrategy.computePriority(queryPlus, segments);
     query = priority.map(query::withPriority).orElse(query);
     Optional<String> lane = laningStrategy.computeLane(queryPlus.withQuery(query), segments);
+    log.info("[%s] lane assigned to [%s] query with [%,d] priority", lane.orElse("default"), query.getType(), priority.orElse(new Integer(0)));
+    final ServiceMetricEvent.Builder builderUsr = ServiceMetricEvent.builder().setFeed("metrics")
+                                                                    .setDimension("lane", lane.orElse("default"))
+                                                                    .setDimension("dataSource", query.getDataSource().getTableNames())
+                                                                    .setDimension("type", query.getType());
+    emitter.emit(builderUsr.build("query/priority", priority.orElse(new Integer(0))));

Review comment:
       ```suggestion
       emitter.emit(builderUsr.build("query/priority", priority.orElse(0)));
   ```




-- 
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] FrankChen021 commented on a change in pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12111:
URL: https://github.com/apache/druid/pull/12111#discussion_r777788626



##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -86,7 +91,34 @@
    * but it is OK in most cases since they will be cleaned up once the query is done.
    */
   private final SetMultimap<String, String> queryDatasources;
+  private final ServiceEmitter emitter;
 
+  public QueryScheduler(
+      int totalNumThreads,
+      QueryPrioritizationStrategy prioritizationStrategy,
+      QueryLaningStrategy laningStrategy,
+      ServerConfig serverConfig,
+      ServiceEmitter emitter
+  )
+  {
+    this.prioritizationStrategy = prioritizationStrategy;
+    this.laningStrategy = laningStrategy;
+    this.queryFutures = Multimaps.synchronizedSetMultimap(HashMultimap.create());
+    this.queryDatasources = Multimaps.synchronizedSetMultimap(HashMultimap.create());
+    // if totalNumThreads is above 0 and less than druid.server.http.numThreads, enforce total limit
+    final boolean limitTotal;
+    if (totalNumThreads > 0 && totalNumThreads < serverConfig.getNumThreads()) {
+      limitTotal = true;
+      this.totalCapacity = totalNumThreads;
+    } else {
+      limitTotal = false;
+      this.totalCapacity = serverConfig.getNumThreads();
+    }
+    this.laneRegistry = BulkheadRegistry.of(getLaneConfigs(limitTotal));
+    this.emitter = emitter;
+  }
+
+  @VisibleForTesting

Review comment:
       Why is this annotation needed? I see the ctor is declared as public




-- 
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] isandeep41 commented on pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
isandeep41 commented on pull request #12111:
URL: https://github.com/apache/druid/pull/12111#issuecomment-1059790195


   Thanks @gianm 


-- 
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] asdf2014 commented on a change in pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #12111:
URL: https://github.com/apache/druid/pull/12111#discussion_r789456961



##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -137,6 +159,12 @@ public void registerQueryFuture(Query<?> query, ListenableFuture<?> future)
     Optional<Integer> priority = prioritizationStrategy.computePriority(queryPlus, segments);
     query = priority.map(query::withPriority).orElse(query);
     Optional<String> lane = laningStrategy.computeLane(queryPlus.withQuery(query), segments);
+    log.info("[%s] lane assigned to [%s] query with [%,d] priority", lane.orElse("default"), query.getType(), priority.orElse(Integer.valueOf(0)));

Review comment:
       ```suggestion
       LOGGER.info("[%s] lane assigned to [%s] query with [%,d] priority", lane.orElse("default"), query.getType(), priority.orElse(Integer.valueOf(0)));
   ```

##########
File path: server/src/main/java/org/apache/druid/server/QueryScheduler.java
##########
@@ -58,6 +62,7 @@
  */
 public class QueryScheduler implements QueryWatcher
 {
+  private static final Logger log = new Logger(QueryScheduler.class);

Review comment:
       ```suggestion
     private static final Logger LOGGER = new Logger(QueryScheduler.class);
   ```

##########
File path: docs/operations/metrics.md
##########
@@ -58,6 +58,7 @@ Available Metrics
 |`query/interrupted/count`|number of queries interrupted due to cancellation.|This metric is only available if the QueryCountStatsMonitor module is included.||
 |`query/timeout/count`|number of timed out queries.|This metric is only available if the QueryCountStatsMonitor module is included.||
 |`query/segments/count`|This metric is not enabled by default. See the `QueryMetrics` Interface for reference regarding enabling this metric. Number of segments that will be touched by the query. In the broker, it makes a plan to distribute the query to realtime tasks and historicals based on a snapshot of segment distribution state. If there are some segments moved after this snapshot is created, certain historicals and realtime tasks can report those segments as missing to the broker. The broker will re-send the query to the new servers that serve those segments after move. In this case, those segments can be counted more than once in this metric.|Varies.|
+|`query/priority`| Assigned lane and priority, only if Laning strategy is enabled. Refer to [Laning strategies](../configuration/index.md#laning-strategies)| lane, dataSource, type|0|

Review comment:
       ```suggestion
   |`query/priority`|Assigned lane and priority, only if Laning strategy is enabled. Refer to [Laning strategies](../configuration/index.md#laning-strategies)|lane, dataSource, type|0|
   ```




-- 
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] FrankChen021 merged pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
FrankChen021 merged pull request #12111:
URL: https://github.com/apache/druid/pull/12111


   


-- 
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] isandeep41 closed pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
isandeep41 closed pull request #12111:
URL: https://github.com/apache/druid/pull/12111


   


-- 
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 #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #12111:
URL: https://github.com/apache/druid/pull/12111#issuecomment-1059399027


   @isandeep41 @FrankChen021 @asdf2014, thank you for getting this PR merged. It seems that Travis has started to fail after this PR is merged (https://app.travis-ci.com/github/apache/druid/builds/247395466). Can you please check the Travis failure? This is a blocker issue since the CI is broken.


-- 
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] FrankChen021 commented on pull request #12111: add a new query laning metrics to visualize lane assignment

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #12111:
URL: https://github.com/apache/druid/pull/12111#issuecomment-1059642931


   Thank you @gianm 


-- 
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