siddharthteotia commented on a change in pull request #6680:
URL: https://github.com/apache/incubator-pinot/pull/6680#discussion_r593513263
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java
##########
@@ -89,12 +92,28 @@ protected IntermediateResultsBlock getNextBlock() {
_futures[i] = _executorService.submit(new TraceRunnable() {
@Override
public void runJob() {
+ ThreadTimer processThreadTimer = new ThreadTimer();
+ processThreadTimer.start();
+
processSegments(threadIndex);
+
+ processThreadTimer.stop();
+ totalWorkerTime.addAndGet(processThreadTimer.getThreadTime());
}
});
}
+ ThreadTimer mergeThreadTimer = new ThreadTimer();
Review comment:
I don't think we should instrument the merge part this way.
We should do that in InstanceResponseOperator and InstanceResponseBlock
because that is the single main thread and all the merge work is happening
under that thread.
So we start the main thread timer in InstanceResponseOperator and stop the
main thread timer in InstanceResponseBlock's constructor and then add up the
timer received from merged intermediate result block. This will cover both
single threaded and multi-threaded code.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
