You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "albertobastos (via GitHub)" <gi...@apache.org> on 2025/01/14 02:45:21 UTC
[PR] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
albertobastos opened a new pull request, #14806:
URL: https://github.com/apache/pinot/pull/14806
https://github.com/apache/pinot/issues/14624
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1916042665
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -277,8 +286,12 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
BrokerResponseNativeV2 brokerResponse = new BrokerResponseNativeV2();
brokerResponse.setResultTable(queryResults.getResultTable());
brokerResponse.setTablesQueried(tableNames);
- // TODO: Add servers queried/responded stats
brokerResponse.setBrokerReduceTimeMs(queryResults.getBrokerReduceTimeMs());
+ // MSE cannot finish if a single queried server did not respond, so we can use the same count for
+ // both the queried and responded stats. Minus one prevents the broker to be included in the count
+ // (it will always be included because of the root of the query plan)
+ brokerResponse.setNumServersQueried(servers.size() - 1);
+ brokerResponse.setNumServersResponded(servers.size() - 1);
Review Comment:
Is this the happy path ?
If something broke, the count will be different and we still want the correct metric to be emitted. So I am not sure why are we explicitly setting them to be same
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1917371935
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -199,8 +201,15 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
}
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
- Set<String> tableNames = queryPlanResult.getTableNames();
+ Set<QueryServerInstance> servers = dispatchableSubPlan.getQueryStageList().stream()
+ .map(DispatchablePlanFragment::getServerInstances)
+ .reduce(new HashSet<QueryServerInstance>(), (all, some) -> {
+ all.addAll(some);
+ return all;
+ });
Review Comment:
I think any of the 3 versions I suggested is easy to read (probably more than a for loop)
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14806:
URL: https://github.com/apache/pinot/pull/14806#issuecomment-2588851334
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14806?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> 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 [(`1a84ec2`)](https://app.codecov.io/gh/apache/pinot/commit/1a84ec24bde026841ea248f6d7f04fc604179c70?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1578 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (1a84ec2). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (1a84ec2) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14806 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14806/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/14806/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/14806/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14806/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/14806/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14806/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/14806/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14806/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14806/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/14806/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14806/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/14806/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/14806/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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14806?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).
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1915421089
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -199,8 +201,15 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
}
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
- Set<String> tableNames = queryPlanResult.getTableNames();
+ Set<QueryServerInstance> servers = dispatchableSubPlan.getQueryStageList().stream()
+ .map(DispatchablePlanFragment::getServerInstances)
+ .reduce(new HashSet<QueryServerInstance>(), (all, some) -> {
+ all.addAll(some);
+ return all;
+ });
Review Comment:
This is correct, but unnecessarily complex to read. What about
What about something like:
```
dispatchableSubPlan.getQueryStageList().stream()
.map(DispatchablePlanFragment::getServerInstances)
.reduce(new HashSet<>(), Sets::union)
.size();
```
Other options (maybe a bit more expensive, but easier to read):
```
dispatchableSubPlan.getQueryStageList().stream()
.flatMap(fragment -> fragment.getServerInstances().stream())
.distinct()
.count();
```
or
```
dispatchableSubPlan.getQueryStageList().stream()
.flatMap(fragment -> fragment.getServerInstances().stream())
.collect(Collectors.toSet())
.size();
```
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1916042665
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -277,8 +286,12 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
BrokerResponseNativeV2 brokerResponse = new BrokerResponseNativeV2();
brokerResponse.setResultTable(queryResults.getResultTable());
brokerResponse.setTablesQueried(tableNames);
- // TODO: Add servers queried/responded stats
brokerResponse.setBrokerReduceTimeMs(queryResults.getBrokerReduceTimeMs());
+ // MSE cannot finish if a single queried server did not respond, so we can use the same count for
+ // both the queried and responded stats. Minus one prevents the broker to be included in the count
+ // (it will always be included because of the root of the query plan)
+ brokerResponse.setNumServersQueried(servers.size() - 1);
+ brokerResponse.setNumServersResponded(servers.size() - 1);
Review Comment:
Is this the happy path ?
If something broke, the count will be different and we still want the correct metric to be emitted. So I am not sure why are we explicitly setting them to be same. They should be set agnostic of whether happy or alternate path was taken
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #14806:
URL: https://github.com/apache/pinot/pull/14806
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1915937215
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -199,8 +201,15 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
}
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
- Set<String> tableNames = queryPlanResult.getTableNames();
+ Set<QueryServerInstance> servers = dispatchableSubPlan.getQueryStageList().stream()
+ .map(DispatchablePlanFragment::getServerInstances)
+ .reduce(new HashSet<QueryServerInstance>(), (all, some) -> {
+ all.addAll(some);
+ return all;
+ });
Review Comment:
I'm not a big fan of functional API (don't really know what's happening behind the scene, and higher overhead), so I'd suggest just using the old fashioned way of creating a set and calling `addAll()`
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1917355976
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -277,8 +286,12 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
BrokerResponseNativeV2 brokerResponse = new BrokerResponseNativeV2();
brokerResponse.setResultTable(queryResults.getResultTable());
brokerResponse.setTablesQueried(tableNames);
- // TODO: Add servers queried/responded stats
brokerResponse.setBrokerReduceTimeMs(queryResults.getBrokerReduceTimeMs());
+ // MSE cannot finish if a single queried server did not respond, so we can use the same count for
+ // both the queried and responded stats. Minus one prevents the broker to be included in the count
+ // (it will always be included because of the root of the query plan)
+ brokerResponse.setNumServersQueried(servers.size() - 1);
+ brokerResponse.setNumServersResponded(servers.size() - 1);
Review Comment:
We can't know how many servers respond. This is the best we can do without actually changing the protocol.
The reason why is that the broker sends stages to the servers it chooses, and then a subset of these servers returns the actual data. Ideally, we could include involved servers in the stats, but the stats do not support set or list types, so we cannot send the different involved segments.
TL;DR: we cannot know how many servers responded in case of an error.
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #14806:
URL: https://github.com/apache/pinot/pull/14806#issuecomment-2594712114
I think a better approach would be to rethink how we are propagating the execution state during the course of query execution. This would be beneficial for debugging and in fact live analysis too.
Imagine a a JOIN + aggregation query that runs for 10secs. As the data is flowing upstream, a tool can be built to look into the live-progress of the query. Built on top of vanilla explain plan, this tool will show the multi-stage plan, which executors are running which stage and the progress of the stage at the operator level.
At any point in time (with some lag), we should be able to drill down into whether a server is waiting on input, if received then how many records received from downstream, how many records produced for upstream so on and so forth.
This will likely require change to the protocol but more importantly will require how the state is measured and communicated.
I don't want to be anal about it but in general, there is no point in emitting a metric which we know may not be always correct to begin with.
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #14806:
URL: https://github.com/apache/pinot/pull/14806#issuecomment-2605680777
> We are never going to send an incorrect metric, but in some cases where we don't know the correct value, the metric won't be included. That is an improvement over the current state where we never set the metric.
Perfect. Thank you
> although it sounds good, I don't have a clear picture of what your suggestion would look like. What seems clear is that significant modifications to the current system would be required. This feature deserves a design document, which we can use to discuss how to implement it and the advantages it could provide.
I agree. It will be quite a bit of surgery. If we think, this may be valuable in the long run, then we can create an issue for now I guess and then evaluate it via design / PEP if / when it is picked up. It could be quite useful from live query observability standpoint but I have not yet thought about the possible overheads.
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1916040427
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -199,8 +201,15 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
}
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
- Set<String> tableNames = queryPlanResult.getTableNames();
+ Set<QueryServerInstance> servers = dispatchableSubPlan.getQueryStageList().stream()
+ .map(DispatchablePlanFragment::getServerInstances)
+ .reduce(new HashSet<QueryServerInstance>(), (all, some) -> {
+ all.addAll(some);
+ return all;
+ });
Review Comment:
+1. I don't find this coding pattern readable.
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1915421089
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -199,8 +201,15 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
}
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
- Set<String> tableNames = queryPlanResult.getTableNames();
+ Set<QueryServerInstance> servers = dispatchableSubPlan.getQueryStageList().stream()
+ .map(DispatchablePlanFragment::getServerInstances)
+ .reduce(new HashSet<QueryServerInstance>(), (all, some) -> {
+ all.addAll(some);
+ return all;
+ });
Review Comment:
nit: This is correct, but unnecessarily complex to read.
What about something like:
```
dispatchableSubPlan.getQueryStageList().stream()
.map(DispatchablePlanFragment::getServerInstances)
.reduce(new HashSet<>(), Sets::union)
.size();
```
Other options (maybe a bit more expensive, but easier to read):
```
dispatchableSubPlan.getQueryStageList().stream()
.flatMap(fragment -> fragment.getServerInstances().stream())
.distinct()
.count();
```
or
```
dispatchableSubPlan.getQueryStageList().stream()
.flatMap(fragment -> fragment.getServerInstances().stream())
.collect(Collectors.toSet())
.size();
```
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "albertobastos (via GitHub)" <gi...@apache.org>.
albertobastos commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1919020149
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -199,8 +201,15 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
}
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
- Set<String> tableNames = queryPlanResult.getTableNames();
+ Set<QueryServerInstance> servers = dispatchableSubPlan.getQueryStageList().stream()
+ .map(DispatchablePlanFragment::getServerInstances)
+ .reduce(new HashSet<QueryServerInstance>(), (all, some) -> {
+ all.addAll(some);
+ return all;
+ });
Review Comment:
I'm gonna betray Gonzalo here 😇
I'm more into using a conventional loop with addAll, too. Using streams here feels a bit overkill for what the code intends to do. Going through the whole array anyway, so I don't see the point of relying into streams.
Actually, that was my first draft and changed it afterwards trying to show off. It obviously didn't go as intended, so conventional loop it is: [de63ddeead](https://github.com/apache/pinot/pull/14806/commits/de63ddeeadbebdfe88910179140046bab66bfcf2)
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #14806:
URL: https://github.com/apache/pinot/pull/14806#issuecomment-2596532599
@siddharthteotia, although it sounds good, I don't have a clear picture of what your suggestion would look like. What seems clear is that significant modifications to the current system would be required. This feature deserves a design document, which we can use to discuss how to implement it and the advantages it could provide.
> I don't want to be anal about it but in general, there is no point in emitting a metric which we know may not be always correct to begin with.
That is never going to be the case, right? We are going to emit the metric if and only if we know that is the correct value. Specifically, whenever a query succeeds, the number of servers that intervened is known and the value matches Alberto's code. When there is an error, `brokerResponse.setNumServersResponded(servers.size() - 1)` won't be called, so we are not going to send that value.
As said, AFAIK, right now, it is impossible to know how many servers intervened on it when a query fails, so this approach is a good (and partial) short-term solution. We are never going to send an incorrect metric, but in some cases where we don't know the correct value, the metric won't be included. That is an improvement over the current state where we never set the metric.
--
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] #bugfix add numServersQueried and numServersResponded to MSQE response [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #14806:
URL: https://github.com/apache/pinot/pull/14806#discussion_r1917872631
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -277,8 +286,12 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
BrokerResponseNativeV2 brokerResponse = new BrokerResponseNativeV2();
brokerResponse.setResultTable(queryResults.getResultTable());
brokerResponse.setTablesQueried(tableNames);
- // TODO: Add servers queried/responded stats
brokerResponse.setBrokerReduceTimeMs(queryResults.getBrokerReduceTimeMs());
+ // MSE cannot finish if a single queried server did not respond, so we can use the same count for
+ // both the queried and responded stats. Minus one prevents the broker to be included in the count
+ // (it will always be included because of the root of the query plan)
+ brokerResponse.setNumServersQueried(servers.size() - 1);
+ brokerResponse.setNumServersResponded(servers.size() - 1);
Review Comment:
Hmm. I think there is an opportunity to make this significantly better. I am sure we will be doing more complex observability for MSE. So, the sooner we fix the debt, the better.
--
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