[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6044: Support for multi-threaded Group By reducer for SQL.

[GitBox <gi...@apache.org>]

mayankshriv commented on a change in pull request #6044:
URL: https://github.com/apache/incubator-pinot/pull/6044#discussion_r492853736



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -161,6 +161,10 @@
     public static final double DEFAULT_BROKER_MIN_RESOURCE_PERCENT_FOR_START = 100.0;
     public static final String CONFIG_OF_ENABLE_QUERY_LIMIT_OVERRIDE = "pinot.broker.enable.query.limit.override";
 
+    // Config for number of threads to use for Broker reduce-phase.
+    public static final String CONFIG_OF_NUM_REDUCE_THREADS = "pinot.broker.num.reduce.threads";
+    public static final int DEFAULT_NUM_REDUCE_THREADS = 1; // TBD: Change to a more appropriate default (eg numCores).

Review comment:
       Actually, on second thought, the default value of `1` is not good, as it will make reduce across concurrent queries as sequential. Moreover, if we add more threads, then it may cause contention in case of high qps use cases.
   
   While we tune this, perhaps the `DEFAULT_NUM_THREADS = 1` should fall back to the current behavior without any executor service and any value `> 1` should use the executor service with specified threads.
   
   Thoughts @kishoreg  @Jackie-Jiang ? 




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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org