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.

Sign up for GitHub

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

Jump to bottom

Add asynchronous metrics on MergeTree tables data volume #17639

Merged
merged 12 commits into from
Dec 22, 2020
Merged

Add asynchronous metrics on MergeTree tables data volume #17639

merged 12 commits into from
Dec 22, 2020

Conversation

ucasfl
Copy link
Collaborator

@ucasfl ucasfl commented Nov 30, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add asynchronous metrics on total amount of rows, bytes and parts in MergeTree tables.
This fix #11714

Detailed description / Documentation draft:

Add metrics on total MergeTree Tables data volume into AsynchronousMetrics.

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Dec 1, 2020
@Akazz Akazz self-assigned this Dec 1, 2020
@ucasfl
fix
ca3f77a 
fix

fix
Akazz
Akazz reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@Akazz Akazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks okay in the rest of it. Although I am a bit uneasy on changing MergeTreeData::getTotalActiveSizeInBytes() and two others b/c they might lose their guarantees of returning consistent values. Let us see how it plays out.

src/Interpreters/AsynchronousMetrics.cpp Outdated Show resolved Hide resolved
@Akazz Akazz added the st-waiting-for-fix We are waiting for fixes in this issue or pull request label Dec 8, 2020
@Akazz Akazz removed the st-waiting-for-fix We are waiting for fixes in this issue or pull request label Dec 14, 2020
@Akazz
Copy link
Contributor

Akazz commented Dec 15, 2020

There is something wrong with test_ttl_replicated::test_ttl_empty_parts. And that is due to changes in MergeTreeData::getPartsCount().

@Akazz Akazz added the st-waiting-for-fix We are waiting for fixes in this issue or pull request label Dec 15, 2020
@ucasfl
Copy link
Collaborator Author

ucasfl commented Dec 19, 2020

Looks ok now.

@ucasfl ucasfl requested a review from Akazz December 19, 2020 11:22
Akazz
Akazz approved these changes Dec 20, 2020
View reviewed changes
Copy link
Contributor

@Akazz Akazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

But we should keep an eye on the behavior of count() since it uses MergeTreeData::getTotalActiveSizeInRows() under the hood, and the method has changed its consistency guarantees with this PR.

src/Interpreters/AsynchronousMetrics.cpp Outdated Show resolved Hide resolved
src/Storages/MergeTree/MergeTreeData.cpp Show resolved Hide resolved
src/Storages/MergeTree/MergeTreeData.cpp Outdated Show resolved Hide resolved
@Akazz
Copy link
Contributor

Akazz commented Dec 20, 2020 •
edited
Loading

All of the failed performance tests are due to "Unexpected Query Duration" and that's irrelevant to this PR

ucasfl and others added 2 commits December 21, 2020 02:04
@Akazz Akazz removed the st-waiting-for-fix We are waiting for fixes in this issue or pull request label Dec 21, 2020
@Akazz Akazz merged commit 4e580f7 into ClickHouse:master Dec 22, 2020
@ucasfl ucasfl mentioned this pull request Feb 10, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akazz Akazz Akazz approved these changes

Assignees

@Akazz Akazz

Labels
pr-feature Pull request with new product feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Asynchronous metrics on data volume
3 participants
@ucasfl @Akazz @robot-clickhouse