vvivekiyer commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2208755527
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -652,6 +654,8 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
long routingEndTimeNs = System.nanoTime();
_brokerMetrics.addPhaseTiming(rawTableName, BrokerQueryPhase.QUERY_ROUTING,
routingEndTimeNs - routingStartTimeNs);
+ // Account the resource used for routing phase
+ Tracing.ThreadAccountantOps.sample();
Review Comment:
Let's add a sample at the end of every phase:
1. Request Compilation
2. Authorization
3. Routing
Also add a comment here as to why sampling for routing is important - with
single threaded segment pruners, this can take resources when there are a lot
of segments.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -371,6 +371,8 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
CompileResult compileResult =
compileRequest(requestId, query, sqlNodeAndOptions, request,
requesterIdentity, requestContext, httpHeaders,
accessControl);
+ // Accounts for resource usage of the compilation phase
+ Tracing.ThreadAccountantOps.sample();
Review Comment:
Can we also make this change for the other handlers?
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -305,6 +306,8 @@ private void applyQueryOptions(QueryContext queryContext) {
@Override
public PlanNode makeSegmentPlanNode(SegmentContext segmentContext,
QueryContext queryContext) {
rewriteQueryContextWithHints(queryContext,
segmentContext.getIndexSegment());
+ // Sample to track usage of query planning
+ Tracing.ThreadAccountantOps.sample();
Review Comment:
Can we use sampleAndCheckInterruption() in all these places to make sure the
thread exists if it's already been interrupted?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -273,6 +274,8 @@ public CompiledQuery compile(String sqlQuery,
SqlNodeAndOptions sqlNodeAndOption
queryNode = sqlNode;
}
RelRoot relRoot = compileQuery(queryNode, plannerContext);
+ // Accounts for resource usage of the compilation phase
+ Tracing.ThreadAccountantOps.sampleMSE();
Review Comment:
setupRunner is called much later before dispatch in
MultistageBrokerRequestHandler. Two questions here:
1. It will not work for multistage handlers.
2. If this is for the single stage handler fallback path, is this really
necessary if we already are sampling after the compilation phase?
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -305,6 +306,8 @@ private void applyQueryOptions(QueryContext queryContext) {
@Override
public PlanNode makeSegmentPlanNode(SegmentContext segmentContext,
QueryContext queryContext) {
rewriteQueryContextWithHints(queryContext,
segmentContext.getIndexSegment());
+ // Sample to track usage of query planning
Review Comment:
We need to track 2 things on the server right?
1. Query Planning
2. Segment Pruning
Can you make sure both are tracked?
--
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]
