-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 MSQ query context maxNumSegments
#16637
Add MSQ query context maxNumSegments
#16637
Conversation
- Default is MAX_INT (unbounded). - When set and if a time chunk contains more number of segments than set in the query context, the MSQ task will fail with TooManySegments fault.
…ents). Also fix formatter.
0246c18
to
6202bdd
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.
Left some comments.
Overall lgtm.
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java
Show resolved
Hide resolved
...s-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/MultiStageQueryContext.java
Outdated
Show resolved
Hide resolved
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.
Changes LGTM!!
…tor-druid into msq_max_num_segments
Thanks for the review, @cryptoe. I renamed the validation function and also included granularity info in the error message. |
This patch:
maxNumSegments
that acts as a guard rail on the maximum number of segments that can be generated by an ingestion task per time chunk. A similar parameter exists in the split hint spec supported by the native engine.Integer.MAX_INT
, butnull
feels more natural.TooManySegmentsInTimeChunkFault
.Most users won't need to set this config, but this will be useful in the following scenarios:
I intentionally left the context parameter and the fault undocumented. We can document it once we have a wider use case. Similarly, no release note is required for this patch.
This PR has: