You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2023/07/01 08:15:41 UTC
[GitHub] [pinot] tibrewalpratik17 opened a new pull request, #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
tibrewalpratik17 opened a new pull request, #11023:
URL: https://github.com/apache/pinot/pull/11023
labels:
- `multistage`
numGroupsLimitReached metric was emitted in v1 but was not getting emitted for v2 queries. Adding that support.
This is also called out as a task in #10781
cc @ankitsultana @walterddr
--
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] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1254927076
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
do we need to track exactly which table triggers the group limit? cuz the flag is global. there's really not much a user can do with per-table insight, correct?
--
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] ankitsultana commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1254955770
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
Yeah you are right it would get called for every stage.
Can we do a general solution instead of emitting only `isNumGroupsLimitReached` metric? You can probably add a new method to ExecutionStatsAggregator and emit all the metrics that are being emitted for v1 right now, and we could call that at the end of the 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
[GitHub] [pinot] codecov-commenter commented on pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11023:
URL: https://github.com/apache/pinot/pull/11023#issuecomment-1615730477
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11023?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11023](https://app.codecov.io/gh/apache/pinot/pull/11023?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1cfc84b) into [master](https://app.codecov.io/gh/apache/pinot/commit/6f2bcde2e1fcee73f81cab24f4054e58d41d6b94?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6f2bcde) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11023 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2193 2139 -54
Lines 118095 115592 -2503
Branches 17884 17583 -301
=========================================
Hits 137 137
+ Misses 117938 115435 -2503
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
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.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [56 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11023/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
[GitHub] [pinot] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1263767555
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
just a quick thought. do we need to handle the "group-limit" metrics emission on broker? cant we do this on the servers and would that make more sense?
- upon checking the group-by reducer code on broker it doesn't actually apply the group limit,
- only place it is applied and trimmed result is on server group-by-operator and combine
- v2 intermediate stage also will apply limit but at that point the "table" info is lost.
does it make sense to
1. directly emit the metrics on server?
2. do not allow group-limit to be apply on intermediate stage?
--
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] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1260162987
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
oh. sorry i completely misunderstood your intent for this PR. so the goal is to emit a metrics not response metadata from broker returned to user?
if that's the case you will have to attach some meaningful rawTableName here. otherwise simply a null is not differentiating for stats purpose.
--
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] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1273764982
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
+ _brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED,
Review Comment:
in this case can we set a multi-stage default table for the metric tag so that we know (1) this is v2, (2) somewhere in the metrics it is reached limit but we don't know which query does 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
[GitHub] [pinot] ankitsultana commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1248823295
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
Say 3 tables were used in a join: `table_1, table_2, table_3`. If `table_2` was the one that had groups limit reached, the current logic will emit a metric for `table_2` and `table_3` since the same `brokerResponse` is written to in the loop.
I don't have context about this piece of code but I see `ExecutionStatsAggregation#setStats` is already emitting a bunch of metrics. I think we can emit this groups limit metric there as well.
The `rawTableName` is set to null right now which skips the metric emission there so we need to think about that.
Maybe we should emit global counters instead of table level counters there?
For Multistage queries, I am not sure if table level metrics make a lot of sense at broker level. My thought is that we can start with global metrics and then later think about table-level metrics.
@walterddr lmk your thoughts.
--
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] tibrewalpratik17 commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1260193410
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
> oh. sorry i completely misunderstood your intent for this PR. so the goal is to emit a metrics not response metadata from broker returned to user?
Yes we want a metric to be emitted based on `isNumGroupsLimitReached` field. We do it for v1 right now.
> if that's the case you will have to attach some meaningful rawTableName here. otherwise simply a null is not differentiating for stats purpose.
As Ankit mentioned that we might emit metric twice if doing it inside the for loop. Should we save the first table we are getting this metric for and just emit based on that?
--
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] tibrewalpratik17 commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1254909661
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
@ankitsultana do you think it will make sense to emit a metric for just `table_2` here and skip for `table_3`. We can keep a track if the metric was emitted in one of the previous stages and skip in subsequent stages. I think this is better than emitting it for tableName as null.
Let me know what do you think? cc @walterddr
--
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] ankitsultana commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1254947757
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
> do we need to track exactly which table triggers the group limit? cuz the flag is global. there's really not much a user can do with per-table insight, correct?
+1. I think we should simply go with global metrics. Table level metrics would be confusing if anything. We could consider adding table-level metrics later.
--
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] tibrewalpratik17 commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1254951999
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
> do we need to track exactly which table triggers the group limit? cuz the flag is global. there's really not much a user can do with per-table insight, correct?
Yeah this makes sense! We can just emit a global level metric in this case.
> I don't have context about this piece of code but I see ExecutionStatsAggregation#setStats is already emitting a bunch of metrics. I think we can emit this groups limit metric there as well.
The rawTableName is set to null right now which skips the metric emission there so we need to think about that.
@ankitsultana I think we cannot reuse this method as at every stage, this is called and we would end up emitting the metric twice again.
I think we can just maintain a flag that if in any one of the stages `isNumGroupsLimitReached` is true, we can emit a metric with `rawTableName` as null in this method itself outside the loop.
@ankitsultana @walterddr does this sound good?
--
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] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1260162987
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
oh. sorry i completely misunderstood your intent for this PR. so the goal is to emit a metrics not response metadata from broker returned to user?
--
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] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1263767555
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
just a quick thought. do we need to handle the "group-limit" metrics emission on broker? cant we do this on the servers and would that make more sense?
- upon checking the group-by reducer code on broker it doesn't actually apply the group limit,
- only place it is applied and trimmed result is on server group-by-operator and combine
- v2 intermediate stage also will apply limit but at that point the "table" info is lost.
does it make sense to directly emit the metrics on server?
--
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] walterddr commented on a diff in pull request #11023: [multistage] Add numGroupsLimitReached metrics to broker v2 queries
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11023:
URL: https://github.com/apache/pinot/pull/11023#discussion_r1273763818
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -247,6 +247,12 @@ private BrokerResponse handleRequest(long requestId, String query, @Nullable Sql
// find a way to split metrics in case of multiple table
String rawTableName = TableNameBuilder.extractRawTableName(tableNames.iterator().next());
entry.getValue().setStageLevelStats(rawTableName, brokerResponseStats, _brokerMetrics);
+
+ // Track number of queries with number of groups limit reached
+ if (brokerResponse.isNumGroupsLimitReached()) {
Review Comment:
ok so the idea is to use default table or a opaque table if the groupLimitExceeded flag is set and unable to determine which table -- the benefit is to compute how many of these actually reached. but not to fine-grain control the signaling on which table.
I am ok with this as long as we are clear with the terminology
--
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