praveenc7 commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2209126214
##########
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:
Currently it is added for SSE and MSE.
`GrpcBrokerRequestHandler`, `SingleConnectionBrokerRequestHandler` extend
this handler for SSE. `TimeSeriesRequestHandler` seems to be custom handler.
For MSE it is added in `QueryEnvironment`
##########
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:
I have added for Request Compilation & Routing. I did consider for
authorization and looked into our latency metric for authorization it is less
than 0.1 ms . So avoided it
##########
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:
Added for Segment pruning
##########
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:
I see the usage of sampleAndCheckInterruption() mostly on broker on merge. I
taught there was reason for doing that specifically on broker.
Is sampleAndCheckInterruption() a more evolved approach we want to move on
all places on server as well?
##########
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:
1. You are right it was setup after compilation, moved it up, take a look if
looks right?
2. This path is touched by MSE handler so it is not added as a fallback
--
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]
