Re: [PR] Reposition query submission spot for adaptive server selection [pinot]
vvivekiyer merged PR #13327: URL: https://github.com/apache/pinot/pull/13327 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Reposition query submission spot for adaptive server selection [pinot]
vvivekiyer commented on code in PR #13327:
URL: https://github.com/apache/pinot/pull/13327#discussion_r1631717268
##
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##
@@ -56,13 +56,17 @@ public AsyncQueryResponse(QueryRouter queryRouter, long
requestId, Set(HashUtil.getHashMapCapacity(numServersQueried));
+_serverRoutingStatsManager = serverRoutingStatsManager;
for (ServerRoutingInstance serverRoutingInstance : serversQueried) {
+ // Record stats related to query submission just before sending the
request. Otherwise, if the response is
+ // received immediately, there's a possibility of updating query
response stats before updating query
+ // submission stats.
+ _serverRoutingStatsManager.recordStatsAfterQuerySubmission(requestId,
serverRoutingInstance.getInstanceId());
Review Comment:
For low latency usecases at high qps, this can cause the issue mentioned in
comments. Hence we update the stats just before submitting.
I've updated the method name to better convey 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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Reposition query submission spot for adaptive server selection [pinot]
Jackie-Jiang commented on code in PR #13327:
URL: https://github.com/apache/pinot/pull/13327#discussion_r1631594204
##
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##
@@ -56,13 +56,17 @@ public AsyncQueryResponse(QueryRouter queryRouter, long
requestId, Set(HashUtil.getHashMapCapacity(numServersQueried));
+_serverRoutingStatsManager = serverRoutingStatsManager;
for (ServerRoutingInstance serverRoutingInstance : serversQueried) {
+ // Record stats related to query submission just before sending the
request. Otherwise, if the response is
+ // received immediately, there's a possibility of updating query
response stats before updating query
+ // submission stats.
+ _serverRoutingStatsManager.recordStatsAfterQuerySubmission(requestId,
serverRoutingInstance.getInstanceId());
Review Comment:
Per the method name, this should be put within either
`markRequestSubmitted()` or `markRequestSent()`
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Reposition query submission spot for adaptive server selection [pinot]
codecov-commenter commented on PR #13327: URL: https://github.com/apache/pinot/pull/13327#issuecomment-2154241788 ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13327?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 `0%` with `2 lines` in your changes missing coverage. Please review. > Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e63e864`)](https://app.codecov.io/gh/apache/pinot/commit/e63e864b582a3bf91a9a97d1b543914161fd5374?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 564 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/pinot/pull/13327?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...pache/pinot/core/transport/AsyncQueryResponse.java](https://app.codecov.io/gh/apache/pinot/pull/13327?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2FAsyncQueryResponse.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvQXN5bmNRdWVyeVJlc3BvbnNlLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #13327 +/- ## = - Coverage 61.75%0.00% -61.76% = Files 2436 2469 +33 Lines133233 136002 +2769 Branches 2063621050 +414 = - Hits 822740-82274 - Misses44911 136002+91091 + Partials 60480 -6048 ``` | [Flag](https://app.codecov.io/gh/apache/pinot/pull/13327/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/13327/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/13327/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/13327/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/13327/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/13327/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/13327/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.63%)` | :arrow_down: | | [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13327/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.75%)` | :arrow_down: | | [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13327/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/13327/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.76%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/apache/pinot/pull/13327/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/
