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
feat: add the number of merge buffers used for druid emitter #16342
base: master
Are you sure you want to change the base?
Conversation
846f7e7
to
eb8a4ba
Compare
@gianm @LakshSingla can you PTAL? |
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.
Thanks for the first contribution!
The code style would be failing on a few files. Checkout
a) https://github.com/apache/druid/blob/master/dev/intellij-setup.md#code-style to setup the editor to lint the code correctly
b) Running mvn checkstyle:checkstyle
would help in confirming if the checkstyle would fail on the PR or not.
Regarding the PR:
a. I think there should be a query/merge/totalBuffers
metric as well.
b. Apart from this, there can be a wait time associated with each query - the total time required to acquire the merge buffers - this should be a simple change given that now we acquire all the merge buffers in a single place
c. There should be tests for the changes made in the PR.
docs/operations/metrics.md
Outdated
@@ -61,6 +61,7 @@ Most metric values reset each emission period, as specified in `druid.monitoring | |||
|`query/success/count`|Number of queries successfully processed.|This metric is only available if the `QueryCountStatsMonitor` module is included.| | | |||
|`query/failed/count`|Number of failed queries.|This metric is only available if the `QueryCountStatsMonitor` module is included.| | | |||
|`query/interrupted/count`|Number of queries interrupted due to cancellation.|This metric is only available if the `QueryCountStatsMonitor` module is included.| | | |||
|`query/merge/buffersUsed`|number of merge buffers allocated to broker while performing groupBy merge queries.|This metric is only available if the `QueryCountStatsMonitor` module is included.| | |
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.
This shouldn't be "allocated to broker". As per my perception, allocated to the broker means the total number of merge buffers present there. Also, there is no such thing as "groupBy merge query".
|`query/merge/buffersUsed`|number of merge buffers allocated to broker while performing groupBy merge queries.|This metric is only available if the `QueryCountStatsMonitor` module is included.| | | |
|`query/merge/buffersUsed`|Number of merge buffers used up to merge the results of group by queries.|This metric is only available if the `QueryCountStatsMonitor` module is included.| | |
public int getUsedBufferCount() | ||
{ | ||
return Integer.MAX_VALUE; | ||
} |
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.
We can probably have the correct implementation for this class, even if it is a test utility.
eb8a4ba
to
fa73e2c
Compare
Thanks for the review @LakshSingla.
I didn't really get what needed to be done here. Can you elaborate more on this? |
Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>
fa73e2c
to
933be1a
Compare
@TessaIO Aplogy for getting back late on this PR. The PR LGTM, can you please address the test failures. My original comment was related to adding more metrics around this area, such as the elapsed time between the query requesting the merge buffer, and the query getting allotted the merge buffer. That can be a part of subsequent PRs. |
Fixes #4345 .
Description
Add the current number of merge buffers used for historical, broker, and real-time.
Release note
Add the current number of merge buffers used for historical, broker, and real-time.
This PR has: