siddharthteotia commented on a change in pull request #6886:
URL: https://github.com/apache/incubator-pinot/pull/6886#discussion_r630471185
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java
##########
@@ -127,7 +127,7 @@ public void runJob() {
// Deregister the main thread and wait for all threads done
phaser.awaitAdvance(phaser.arriveAndDeregister());
}
- CombineOperatorUtils.setExecutionStatistics(mergedBlock, _operators,
totalWorkerThreadCpuTimeNs.get());
+ CombineOperatorUtils.setExecutionStatistics(mergedBlock, _operators,
totalWorkerThreadCpuTimeNs.get(), _numThreads);
Review comment:
So, let's change _numThreads to numTasks or something more meaningful
We have 2 cases
- GroupBy passes _numTasks as numOperators
- Selection and everyone else passes _numTasks as a factor of number of
cores see function `public static int getNumThreadsForQuery(int numOperators)`
in CombineOperatorUtils
case 1 - query on 100 segments with 8 threads in worker executor service
- group by - _numTasks = 100, _numOperators = 100, worker threads = 8
- selection and everyone else - _numTasks = 8, _numOperators = 100, worker
threads = 8
case 2 - query on 4 segments with 8 threads in worker executor service
- group by - _numTasks = 4, _numOperators = 4, worker threads = 8
- selection and everyone else - _numTasks = 4, _numOperators = 4, worker
threads = 8
Let's have good tests for all these. You can also run
OfflineClusterIntegrationTest in debug mode and see how _numOperators,
_numTasks and numWorkerThreads look like
--
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
