You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "vvivekiyer (via GitHub)" <gi...@apache.org> on 2023/11/10 02:17:47 UTC
[PR] Add query option override for Broker MinGroupTrimSize [pinot]
vvivekiyer opened a new pull request, #11984:
URL: https://github.com/apache/pinot/pull/11984
1. Added a queryOption override `minBrokerGroupTrimSize` for config `pinot.broker.min.group.trim.size`
2. Reused the generic queryOption override `groupTrimThreshold` at the broker reduce stage as well
Note: For existing users of groupTrimThreshold queryOption, they can see trimming kicking in going forward. I'm open to suggestions if we want to decouple and create a separate queryOption for broker.
--
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] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #11984:
URL: https://github.com/apache/pinot/pull/11984#discussion_r1393784447
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -173,6 +173,15 @@ public static Integer getMinServerGroupTrimSize(Map<String, String> queryOptions
return minServerGroupTrimSizeString != null ? Integer.parseInt(minServerGroupTrimSizeString) : null;
}
+ @Nullable
+ public static Integer getMinBrokerGroupTrimSize(Map<String, String> queryOptions) {
+ if (queryOptions == null) {
+ return null;
+ }
Review Comment:
Moved this check into (Streaming/Broker)ReduceService. This check is needed because unlike servers (where queryOption cannot be null because of timeout,etc), it can be null for brokers.
--
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] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on PR #11984:
URL: https://github.com/apache/pinot/pull/11984#issuecomment-1813689237
Added to docs.
--
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] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11984:
URL: https://github.com/apache/pinot/pull/11984#discussion_r1393017745
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -206,6 +215,9 @@ public static Integer getMaxInitialResultHolderCapacity(Map<String, String> quer
@Nullable
public static Integer getGroupTrimThreshold(Map<String, String> queryOptions) {
+ if (queryOptions == null) {
+ return null;
+ }
Review Comment:
Same here
```suggestion
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -173,6 +173,15 @@ public static Integer getMinServerGroupTrimSize(Map<String, String> queryOptions
return minServerGroupTrimSizeString != null ? Integer.parseInt(minServerGroupTrimSizeString) : null;
}
+ @Nullable
+ public static Integer getMinBrokerGroupTrimSize(Map<String, String> queryOptions) {
+ if (queryOptions == null) {
+ return null;
+ }
Review Comment:
Let's follow the same convention by assuming `queryOptions` is not null
```suggestion
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BaseReduceService.java:
##########
@@ -50,15 +50,15 @@ public abstract class BaseReduceService {
protected final ExecutorService _reduceExecutorService;
protected final int _maxReduceThreadsPerQuery;
- protected final int _groupByTrimThreshold;
- protected final int _minGroupTrimSize;
+ protected final int _groupByTrimThresholdCfg;
Review Comment:
(minor) No need to change this. Member variable indicates it is not from 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
Re: [PR] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11984:
URL: https://github.com/apache/pinot/pull/11984#discussion_r1395053217
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BaseReduceService.java:
##########
@@ -50,15 +50,15 @@ public abstract class BaseReduceService {
protected final ExecutorService _reduceExecutorService;
protected final int _maxReduceThreadsPerQuery;
- protected final int _groupByTrimThreshold;
- protected final int _minGroupTrimSize;
+ protected final int _groupByTrimThresholdCfg;
Review Comment:
Is this comment missed?
--
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] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11984:
URL: https://github.com/apache/pinot/pull/11984#issuecomment-1804999234
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11984?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11984](https://app.codecov.io/gh/apache/pinot/pull/11984?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a9e30d5) into [master](https://app.codecov.io/gh/apache/pinot/commit/2beb9a4938d7d7c9481fd2546075e2a2475fe0ec?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2beb9a4) will **decrease** coverage by `61.62%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11984 +/- ##
=============================================
- Coverage 61.61% 0.00% -61.62%
=============================================
Files 2385 2309 -76
Lines 129214 125476 -3738
Branches 20003 19450 -553
=============================================
- Hits 79613 0 -79613
- Misses 43801 125476 +81675
+ Partials 5800 0 -5800
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11984/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/11984/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/11984/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/11984/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/11984/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/11984/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/11984/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.48%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/11984/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.62%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/11984/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%> (-27.59%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11984/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.62%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11984/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/11984/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/11984/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/11984?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/11984?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (-31.00%)` | :arrow_down: |
| [...e/pinot/common/utils/config/QueryOptionsUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11984?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvY29uZmlnL1F1ZXJ5T3B0aW9uc1V0aWxzLmphdmE=) | `0.00% <0.00%> (-60.53%)` | :arrow_down: |
| [...e/pinot/core/query/reduce/BrokerReduceService.java](https://app.codecov.io/gh/apache/pinot/pull/11984?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQnJva2VyUmVkdWNlU2VydmljZS5qYXZh) | `0.00% <0.00%> (-70.77%)` | :arrow_down: |
| [...inot/core/query/reduce/StreamingReduceService.java](https://app.codecov.io/gh/apache/pinot/pull/11984?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvU3RyZWFtaW5nUmVkdWNlU2VydmljZS5qYXZh) | `0.00% <0.00%> (-26.93%)` | :arrow_down: |
... and [1991 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11984/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
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] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11984:
URL: https://github.com/apache/pinot/pull/11984
--
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] Add query option override for Broker MinGroupTrimSize [pinot]
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on PR #11984:
URL: https://github.com/apache/pinot/pull/11984#issuecomment-1811961790
I'll update docs once this PR is merged.
--
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