You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/11/02 17:38:08 UTC
[PR] Ask server to directly return final result for queries hitting single server [pinot]
Jackie-Jiang opened a new pull request, #11938:
URL: https://github.com/apache/pinot/pull/11938
#9304 introduced the query option `serverReturnFinalResult` which can be used when no broker reduce is required, and ask server to directly return the final aggregation result.
This PR extends that by automatically applying this query option when only one server is queried.
This PR also contains some bugfix for server response size (#11710) to prevent NPE when table config is not loaded yet
--
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] Ask server to directly return final result for queries hitting single server [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11938:
URL: https://github.com/apache/pinot/pull/11938#discussion_r1382052784
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -684,22 +685,38 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
return new BrokerResponseNative(exceptions);
}
- // Set the maximum serialized response size per server
+ // Set the maximum serialized response size per server, and ask server to directly return final response when only
+ // one server is queried
int numServers = 0;
if (offlineRoutingTable != null) {
numServers += offlineRoutingTable.size();
}
if (realtimeRoutingTable != null) {
numServers += realtimeRoutingTable.size();
}
-
if (offlineBrokerRequest != null) {
- setMaxServerResponseSizeBytes(numServers, offlineBrokerRequest.getPinotQuery().getQueryOptions(),
- offlineTableConfig);
+ Map<String, String> queryOptions = offlineBrokerRequest.getPinotQuery().getQueryOptions();
+ setMaxServerResponseSizeBytes(numServers, queryOptions, offlineTableConfig);
+ // Set the query option to directly return final result for single server query unless it is explicitly disabled
+ if (numServers == 1) {
+ if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
+ && offlineBrokerRequest != serverBrokerRequest) {
+ serverBrokerRequest.getPinotQuery().getQueryOptions()
+ .put(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
+ }
+ }
}
if (realtimeBrokerRequest != null) {
- setMaxServerResponseSizeBytes(numServers, realtimeBrokerRequest.getPinotQuery().getQueryOptions(),
- realtimeTableConfig);
+ Map<String, String> queryOptions = realtimeBrokerRequest.getPinotQuery().getQueryOptions();
+ setMaxServerResponseSizeBytes(numServers, queryOptions, realtimeTableConfig);
+ // Set the query option to directly return final result for single server query unless it is explicitly disabled
+ if (numServers == 1) {
+ if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
+ && offlineBrokerRequest != serverBrokerRequest) {
Review Comment:
Good catch! Let me add some comment explaining this
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -684,22 +685,38 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
return new BrokerResponseNative(exceptions);
}
- // Set the maximum serialized response size per server
+ // Set the maximum serialized response size per server, and ask server to directly return final response when only
+ // one server is queried
int numServers = 0;
if (offlineRoutingTable != null) {
numServers += offlineRoutingTable.size();
}
if (realtimeRoutingTable != null) {
numServers += realtimeRoutingTable.size();
}
-
if (offlineBrokerRequest != null) {
- setMaxServerResponseSizeBytes(numServers, offlineBrokerRequest.getPinotQuery().getQueryOptions(),
- offlineTableConfig);
+ Map<String, String> queryOptions = offlineBrokerRequest.getPinotQuery().getQueryOptions();
+ setMaxServerResponseSizeBytes(numServers, queryOptions, offlineTableConfig);
+ // Set the query option to directly return final result for single server query unless it is explicitly disabled
+ if (numServers == 1) {
+ if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
+ && offlineBrokerRequest != serverBrokerRequest) {
+ serverBrokerRequest.getPinotQuery().getQueryOptions()
+ .put(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
+ }
+ }
}
if (realtimeBrokerRequest != null) {
- setMaxServerResponseSizeBytes(numServers, realtimeBrokerRequest.getPinotQuery().getQueryOptions(),
- realtimeTableConfig);
+ Map<String, String> queryOptions = realtimeBrokerRequest.getPinotQuery().getQueryOptions();
+ setMaxServerResponseSizeBytes(numServers, queryOptions, realtimeTableConfig);
+ // Set the query option to directly return final result for single server query unless it is explicitly disabled
+ if (numServers == 1) {
+ if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
+ && offlineBrokerRequest != serverBrokerRequest) {
Review Comment:
Good catch! Let me also add some comment explaining this
--
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] Ask server to directly return final result for queries hitting single server [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11938:
URL: https://github.com/apache/pinot/pull/11938#discussion_r1381926384
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -684,22 +685,38 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
return new BrokerResponseNative(exceptions);
}
- // Set the maximum serialized response size per server
+ // Set the maximum serialized response size per server, and ask server to directly return final response when only
+ // one server is queried
int numServers = 0;
if (offlineRoutingTable != null) {
numServers += offlineRoutingTable.size();
}
if (realtimeRoutingTable != null) {
numServers += realtimeRoutingTable.size();
}
-
if (offlineBrokerRequest != null) {
- setMaxServerResponseSizeBytes(numServers, offlineBrokerRequest.getPinotQuery().getQueryOptions(),
- offlineTableConfig);
+ Map<String, String> queryOptions = offlineBrokerRequest.getPinotQuery().getQueryOptions();
+ setMaxServerResponseSizeBytes(numServers, queryOptions, offlineTableConfig);
+ // Set the query option to directly return final result for single server query unless it is explicitly disabled
+ if (numServers == 1) {
+ if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
+ && offlineBrokerRequest != serverBrokerRequest) {
+ serverBrokerRequest.getPinotQuery().getQueryOptions()
+ .put(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
+ }
+ }
}
if (realtimeBrokerRequest != null) {
- setMaxServerResponseSizeBytes(numServers, realtimeBrokerRequest.getPinotQuery().getQueryOptions(),
- realtimeTableConfig);
+ Map<String, String> queryOptions = realtimeBrokerRequest.getPinotQuery().getQueryOptions();
+ setMaxServerResponseSizeBytes(numServers, queryOptions, realtimeTableConfig);
+ // Set the query option to directly return final result for single server query unless it is explicitly disabled
+ if (numServers == 1) {
+ if (queryOptions.putIfAbsent(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true") == null
+ && offlineBrokerRequest != serverBrokerRequest) {
Review Comment:
realtimeBrokerRequest?
--
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] Ask server to directly return final result for queries hitting single server [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11938:
URL: https://github.com/apache/pinot/pull/11938
--
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] Ask server to directly return final result for queries hitting single server [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11938:
URL: https://github.com/apache/pinot/pull/11938#issuecomment-1791313477
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11938?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11938](https://app.codecov.io/gh/apache/pinot/pull/11938?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e4fa9bf) into [master](https://app.codecov.io/gh/apache/pinot/commit/fee11d6dc5678600c021a5993ead4b9c5102c76c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fee11d6) will **decrease** coverage by `61.42%`.
> Report is 1 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11938 +/- ##
=============================================
- Coverage 61.41% 0.00% -61.42%
=============================================
Files 2378 2302 -76
Lines 128865 125121 -3744
Branches 19927 19372 -555
=============================================
- Hits 79143 0 -79143
- Misses 44001 125121 +81120
+ Partials 5721 0 -5721
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11938/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/11938/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/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11938/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/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11938/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/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.28%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.41%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/11938/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/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.42%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11938/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/11938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://app.codecov.io/gh/apache/pinot/pull/11938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (-28.00%)` | :arrow_down: |
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-46.00%)` | :arrow_down: |
... and [1984 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11938/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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