You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "praveenc7 (via GitHub)" <gi...@apache.org> on 2025/06/20 16:56:15 UTC
[PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
praveenc7 opened a new pull request, #16164:
URL: https://github.com/apache/pinot/pull/16164
## Summary
We have seen in few cases where lack of sampling has contributed to queries not getting killed sooner, adding sampling for
### Broker
1. Compilation
2. Routing
### Server
1. Planning phase
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2219891142
##########
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, since it can be expensive for large segment lists.
+ Tracing.ThreadAccountantOps.sampleAndCheckInterruption();
Review Comment:
I understand that consolidating the sampling into a single location can improve readability, but in this case it would come at the cost of precision—because you’d only capture metrics once, after the entire operation completes. For example, we have several pruner and ideally need to record a sample after each one to maintain accuracy (and the same applies to planning).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2219787374
##########
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 authorization. The idea is that if there's a bug/deployment issue in this phase, we catch it.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2216404250
##########
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:
Routing was closer to the phase calculation moved compilation as well
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer merged PR #16164:
URL: https://github.com/apache/pinot/pull/16164
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2219892181
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -298,7 +298,6 @@ protected BrokerResponse handleRequestThrowing(long requestId, String query, Sql
try (QueryEnvironment.CompiledQuery compiledQuery =
compileQuery(requestId, query, sqlNodeAndOptions, httpHeaders, queryTimer)) {
-
Review Comment:
This was because of a bad merge with master, fixed it
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2210872357
##########
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:
You should see it used on server as well in some of the operators. The only difference is one blindly samples. While the other checks for interruption and throws exception before sampling.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2210874160
##########
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:
(nit) Prefer moving the sampling to the location where we compute phase timing for each phase. That way it's evident that we want to sample each broker phase.
##########
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:
It doesn't look right to me. The QueryEnv.compile() happens in an async call with a separate threadpool. We might have to find another way to account for these async calls in MSE.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16164:
URL: https://github.com/apache/pinot/pull/16164#issuecomment-2992379927
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16164?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `66.66667%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 33.35%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ec7a2ef`)](https://app.codecov.io/gh/apache/pinot/commit/ec7a2ef9c8fe98270279117d8d2c9545ee22198e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 306 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16164?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pinot/core/plan/maker/InstancePlanMakerImplV2.java](https://app.codecov.io/gh/apache/pinot/pull/16164?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fplan%2Fmaker%2FInstancePlanMakerImplV2.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL21ha2VyL0luc3RhbmNlUGxhbk1ha2VySW1wbFYyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (ec7a2ef). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (ec7a2ef) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|0|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|1|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16164 +/- ##
=============================================
- Coverage 62.90% 33.35% -29.56%
+ Complexity 1386 659 -727
=============================================
Files 2867 2953 +86
Lines 163354 169805 +6451
Branches 24952 25962 +1010
=============================================
- Hits 102755 56631 -46124
- Misses 52847 108629 +55782
+ Partials 7752 4545 -3207
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.35% <66.66%> (-29.48%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.35% <66.66%> (-29.56%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.35% <66.66%> (-29.55%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16164/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.35% <66.66%> (-0.23%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16164?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Resource Isolation] Additonal Sampling for Broker and Server [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #16164:
URL: https://github.com/apache/pinot/pull/16164#discussion_r2219869399
##########
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, since it can be expensive for large segment lists.
+ Tracing.ThreadAccountantOps.sampleAndCheckInterruption();
Review Comment:
Keep all the sampling in a single place without moving it inside the planning/pruning classes.. That way it's easier to reason about it.
For example: can we add both the pruning and planning sampling in ServerQueryExecutorV1Impl.
- Pruning sampling in `executeInternal()` after the selectedSegmentsInfo is computed.
- Planning in `executeQuery` before the `planCombineQuery` is called.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -298,7 +298,6 @@ protected BrokerResponse handleRequestThrowing(long requestId, String query, Sql
try (QueryEnvironment.CompiledQuery compiledQuery =
compileQuery(requestId, query, sqlNodeAndOptions, httpHeaders, queryTimer)) {
-
Review Comment:
(nit) remove spurious changes.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org