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 behavior should be:
   - If config not explicitly specified, then preserve current behavior without 
executor service, or perhaps using `MoreExecutors.newDirectExecutorService()` 
that uses the calling thread to execute the Runnable.
   - If config specified, use executor service with num threads specified in 
the config.
   
   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:
us...@infra.apache.org



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

Reply via email to