You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jadami10 (via GitHub)" <gi...@apache.org> on 2023/04/11 00:53:03 UTC
[GitHub] [pinot] jadami10 opened a new pull request, #10589: no error metric for queries where all segments are pruned
jadami10 opened a new pull request, #10589:
URL: https://github.com/apache/pinot/pull/10589
this is a small bugfix.
Currently, when all segments are pruned at the broker level, we treat it as if no servers could be found. But we also increment the "no servers found" metric. This isn't actually true in this case, so in this PR we avoid that error log and that metric, and return the broker native response instead. This also fixes the other small bug the `EXPLAIN PLAN FOR` when everything is pruned was also just returning the purely empty response rather than the "broker only explanation".
I tested this by updating the airlineStats table to use time pruning. I left that change in the json because I figured it's nice to have 1 table with time pruning. But happy to revert. Otherwise, I couldn't find any unit tests around this class.
--
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
[GitHub] [pinot] jadami10 commented on pull request #10589: no error metric for queries where all segments are pruned
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on PR #10589:
URL: https://github.com/apache/pinot/pull/10589#issuecomment-1511836576
@Jackie-Jiang thoughts here?
--
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
[GitHub] [pinot] swaminathanmanish commented on a diff in pull request #10589: no error metric for queries where all segments are pruned
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10589:
URL: https://github.com/apache/pinot/pull/10589#discussion_r1162293068
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -728,6 +723,21 @@ private BrokerResponseNative handleRequest(long requestId, String query,
}
}
+ private BrokerResponseNative getEmptyBrokerOnlyResponse(long requestId, String query,
+ RequesterIdentity requesterIdentity, RequestContext requestContext, PinotQuery pinotQuery, String tableName) {
+ if (pinotQuery.isExplain()) {
+ // EXPLAIN PLAN results to show that query is evaluated exclusively by Broker.
+ return BrokerResponseNative.BROKER_ONLY_EXPLAIN_PLAN_OUTPUT;
+ }
+
+ // Send empty response since we don't need to evaluate either offline or realtime request.
+ BrokerResponseNative brokerResponse = BrokerResponseNative.empty();
Review Comment:
Would adding to **_brokerMetrics** here be useful 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
[GitHub] [pinot] Jackie-Jiang merged pull request #10589: no error metric for queries where all segments are pruned
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10589:
URL: https://github.com/apache/pinot/pull/10589
--
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
[GitHub] [pinot] jadami10 commented on a diff in pull request #10589: no error metric for queries where all segments are pruned
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #10589:
URL: https://github.com/apache/pinot/pull/10589#discussion_r1162198082
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -618,9 +607,15 @@ private BrokerResponseNative handleRequest(long requestId, String query,
}
if (offlineBrokerRequest == null && realtimeBrokerRequest == null) {
- LOGGER.info("No server found for request {}: {}", requestId, query);
- _brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.NO_SERVER_FOUND_EXCEPTIONS, 1);
- return new BrokerResponseNative(exceptions);
+ if (!exceptions.isEmpty()) {
+ LOGGER.info("No server found for request {}: {}", requestId, query);
+ _brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.NO_SERVER_FOUND_EXCEPTIONS, 1);
+ return new BrokerResponseNative(exceptions);
+ } else {
+ // When all segments have been pruned, we can just return an empty response.
+ return getEmptyBrokerOnlyResponse(requestId, query, requesterIdentity, requestContext, pinotQuery,
Review Comment:
we have `setNumSegmentsPrunedByBroker`, but we haven't been setting that here. I imagine we could, but I feel like that might be a bigger semantic change.
--
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
[GitHub] [pinot] jadami10 commented on a diff in pull request #10589: no error metric for queries where all segments are pruned
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #10589:
URL: https://github.com/apache/pinot/pull/10589#discussion_r1162813724
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -728,6 +723,21 @@ private BrokerResponseNative handleRequest(long requestId, String query,
}
}
+ private BrokerResponseNative getEmptyBrokerOnlyResponse(long requestId, String query,
+ RequesterIdentity requesterIdentity, RequestContext requestContext, PinotQuery pinotQuery, String tableName) {
+ if (pinotQuery.isExplain()) {
+ // EXPLAIN PLAN results to show that query is evaluated exclusively by Broker.
+ return BrokerResponseNative.BROKER_ONLY_EXPLAIN_PLAN_OUTPUT;
+ }
+
+ // Send empty response since we don't need to evaluate either offline or realtime request.
+ BrokerResponseNative brokerResponse = BrokerResponseNative.empty();
Review Comment:
adding how exactly?
--
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
[GitHub] [pinot] jadami10 commented on a diff in pull request #10589: no error metric for queries where all segments are pruned
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #10589:
URL: https://github.com/apache/pinot/pull/10589#discussion_r1163353297
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -728,6 +723,21 @@ private BrokerResponseNative handleRequest(long requestId, String query,
}
}
+ private BrokerResponseNative getEmptyBrokerOnlyResponse(long requestId, String query,
+ RequesterIdentity requesterIdentity, RequestContext requestContext, PinotQuery pinotQuery, String tableName) {
+ if (pinotQuery.isExplain()) {
+ // EXPLAIN PLAN results to show that query is evaluated exclusively by Broker.
+ return BrokerResponseNative.BROKER_ONLY_EXPLAIN_PLAN_OUTPUT;
+ }
+
+ // Send empty response since we don't need to evaluate either offline or realtime request.
+ BrokerResponseNative brokerResponse = BrokerResponseNative.empty();
Review Comment:
ah I see. I'm leaning no for now. The initial case where we return this empty response was when the query always returned false. And that one didn't increment any metric. As a Pinot operator, I'm not sure what I would do with this metric.
Maybe the bigger is that we have several case where we return early, but we don't actually log query.
--
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
[GitHub] [pinot] swaminathanmanish commented on a diff in pull request #10589: no error metric for queries where all segments are pruned
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10589:
URL: https://github.com/apache/pinot/pull/10589#discussion_r1162293068
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -728,6 +723,21 @@ private BrokerResponseNative handleRequest(long requestId, String query,
}
}
+ private BrokerResponseNative getEmptyBrokerOnlyResponse(long requestId, String query,
+ RequesterIdentity requesterIdentity, RequestContext requestContext, PinotQuery pinotQuery, String tableName) {
+ if (pinotQuery.isExplain()) {
+ // EXPLAIN PLAN results to show that query is evaluated exclusively by Broker.
+ return BrokerResponseNative.BROKER_ONLY_EXPLAIN_PLAN_OUTPUT;
+ }
+
+ // Send empty response since we don't need to evaluate either offline or realtime request.
+ BrokerResponseNative brokerResponse = BrokerResponseNative.empty();
Review Comment:
Would adding **_brokerMetrics** here be useful 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
[GitHub] [pinot] codecov-commenter commented on pull request #10589: no error metric for queries where all segments are pruned
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10589:
URL: https://github.com/apache/pinot/pull/10589#issuecomment-1502584758
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10589?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10589](https://codecov.io/gh/apache/pinot/pull/10589?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (75e1817) into [master](https://codecov.io/gh/apache/pinot/commit/cde82bb88377751f7ce1fe73a8231a37fc7fdcd6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cde82bb) will **decrease** coverage by `8.37%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10589 +/- ##
============================================
- Coverage 68.99% 60.62% -8.37%
+ Complexity 6488 6088 -400
============================================
Files 2106 2090 -16
Lines 112970 112541 -429
Branches 17014 16959 -55
============================================
- Hits 77943 68231 -9712
- Misses 29543 39295 +9752
+ Partials 5484 5015 -469
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `24.12% <0.00%> (+0.01%)` | :arrow_up: |
| unittests1 | `67.96% <ø> (+0.03%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10589?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/10589?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `59.18% <0.00%> (-10.08%)` | :arrow_down: |
... and [304 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10589/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] swaminathanmanish commented on a diff in pull request #10589: no error metric for queries where all segments are pruned
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #10589:
URL: https://github.com/apache/pinot/pull/10589#discussion_r1162829792
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -728,6 +723,21 @@ private BrokerResponseNative handleRequest(long requestId, String query,
}
}
+ private BrokerResponseNative getEmptyBrokerOnlyResponse(long requestId, String query,
+ RequesterIdentity requesterIdentity, RequestContext requestContext, PinotQuery pinotQuery, String tableName) {
+ if (pinotQuery.isExplain()) {
+ // EXPLAIN PLAN results to show that query is evaluated exclusively by Broker.
+ return BrokerResponseNative.BROKER_ONLY_EXPLAIN_PLAN_OUTPUT;
+ }
+
+ // Send empty response since we don't need to evaluate either offline or realtime request.
+ BrokerResponseNative brokerResponse = BrokerResponseNative.empty();
Review Comment:
oh, I meant adding new metric to BrokerMeter to track this case, if its useful.
--
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