-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add query option override for Broker MinGroupTrimSize #11984
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #11984 +/- ##
==========================================
Coverage 61.61% 61.61%
- Complexity 207 1150 +943
==========================================
Files 2385 2385
Lines 129214 129266 +52
Branches 20003 20014 +11
==========================================
+ Hits 79613 79649 +36
- Misses 43801 43815 +14
- Partials 5800 5802 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
a9e30d5
to
b538cc7
Compare
b538cc7
to
77fd083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
We may keep the same group trim threshold
the same across the query lifecycle. Ideally we should also use the same group trim size
everywhere, but we kept them separate for backward compatibility where segment level trim is off by default. We may consider unifying them in the future
if (queryOptions == null) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the same convention by assuming queryOptions
is not null
if (queryOptions == null) { | |
return null; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BaseReduceService.java
Outdated
Show resolved
Hide resolved
I'll update docs once this PR is merged. |
ffccff7
to
f9675a5
Compare
Added query override option to docs. |
minBrokerGroupTrimSize
for configpinot.broker.min.group.trim.size
groupTrimThreshold
at the broker reduce stage as wellNote: 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.