siddharthteotia commented on a change in pull request #6886:
URL: https://github.com/apache/incubator-pinot/pull/6886#discussion_r629764516
##########
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:
This is actually incorrect.
We basically want the number of query execution threads. The _numThreads
argument in BaseCombineOperator is misleading (it always has been). The code
goes over it in the loop and submits as many async tasks to the executor as the
_numThreads. That doesn't really mean that executor service will use those many
threads.
You might submit 100 tasks to an executor service of fixed pool of 8
threads. The threads will be 8.
The server side query execution executor service is created during server
startup and the number of threads are determined then and they remain the same
since all queries share the executor service. They are referred to as (PQW) or
worker threads. Each query gets a single PQR (runner thread) and 1 or more
worker (PQW) threads. So, we need to use PQW + PQR here to get the actual
number of threads executing the query. Let's check the code where executor
service is created and find out the number.
--
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
