Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TessaIO
Copy link

@TessaIO TessaIO commented Apr 27, 2024

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:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@TessaIO
Copy link
Author

TessaIO commented Apr 29, 2024

@gianm @LakshSingla can you PTAL?

Copy link
Contributor

@LakshSingla LakshSingla left a 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.

@@ -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.| |
Copy link
Contributor

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".

Suggested change
|`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.| |

Comment on lines 152 to 155
public int getUsedBufferCount()
{
return Integer.MAX_VALUE;
}
Copy link
Contributor

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.

@TessaIO TessaIO force-pushed the feat-add-merge-buffers-metrics branch from eb8a4ba to fa73e2c Compare May 1, 2024 15:13
@TessaIO
Copy link
Author

TessaIO commented May 1, 2024

Thanks for the review @LakshSingla.
I addressed all of your comments. PTAL when you have time.

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

I didn't really get what needed to be done here. Can you elaborate more on this?

@TessaIO TessaIO requested a review from LakshSingla May 1, 2024 15:14
Signed-off-by: TessaIO <ahmedgrati1999@gmail.com>
@TessaIO TessaIO force-pushed the feat-add-merge-buffers-metrics branch from fa73e2c to 933be1a Compare May 1, 2024 15:15
@LakshSingla
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics for merge buffer usage
3 participants