yashmayya commented on code in PR #19816:
URL: https://github.com/apache/pinot/pull/19816#discussion_r4286643993
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -1009,6 +1009,21 @@ private Collection<PlanNode>
requestPhysicalPlan(DispatchablePlanFragment fragme
return stagePlans;
}
+ @VisibleForTesting
+ static void mergeStageStats(BrokerResponseNativeV2 brokerResponse,
+ List<MultiStageQueryStats.StageStats.Closed> queryStats) {
+ for (MultiStageQueryStats.StageStats.Closed stageStats : queryStats) {
+ if (stageStats != null) { // for example pipeline breaker may not have
stats
+ stageStats.forEach((type, stats) -> type.mergeInto(brokerResponse,
stats));
+ if (!stageStats.isEmpty()) {
+ // The stage root's allocation covers all the operators of its op
chain
Review Comment:
Thanks @xiangfu0. Done in c807627e53: the comment now points to
`MultiStageOperator.Type#mergeInto` for the leaf threads and the pipeline
breakers.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -1009,6 +1009,21 @@ private Collection<PlanNode>
requestPhysicalPlan(DispatchablePlanFragment fragme
return stagePlans;
}
+ @VisibleForTesting
+ static void mergeStageStats(BrokerResponseNativeV2 brokerResponse,
+ List<MultiStageQueryStats.StageStats.Closed> queryStats) {
+ for (MultiStageQueryStats.StageStats.Closed stageStats : queryStats) {
+ if (stageStats != null) { // for example pipeline breaker may not have
stats
+ stageStats.forEach((type, stats) -> type.mergeInto(brokerResponse,
stats));
+ if (!stageStats.isEmpty()) {
+ // The stage root's allocation covers all the operators of its op
chain
+ brokerResponse.addTotalMemAllocatedBytes(
+
stageStats.getLastOperatorStats().getUnsafe("ALLOCATED_MEMORY_BYTES", 0L));
Review Comment:
Yes, this is intentional. The stage roots have different stat key types
(`MAILBOX_SEND`, and `MAILBOX_RECEIVE` for stage 0 on the broker), and
`OpChainSchedulerService` reads the same key by name for its allocation meter.
I added the reason to the comment in c807627e53.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -422,6 +422,8 @@ public void mergeInto(BrokerResponseNativeV2 response,
StatMap<?> map) {
@SuppressWarnings("unchecked")
StatMap<LeafOperator.StatKey> stats = (StatMap<LeafOperator.StatKey>)
map;
response.mergeMaxRowsInOperator(stats.getLong(LeafOperator.StatKey.EMITTED_ROWS));
+ // The single-stage threads of the leaf are not covered by the stage
root's allocation
+
response.addTotalMemAllocatedBytes(stats.getLong(LeafOperator.StatKey.THREAD_MEM_ALLOCATED_BYTES));
Review Comment:
Done in c807627e53: the leaf and pipeline-breaker comments now point to
`MultiStageBrokerRequestHandler#mergeStageStats`, so each side names the other.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
