-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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 MemoryTracker for the background tasks [Resubmit] #48787
Conversation
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.
Diff looks OK.
Some tests are failed.
This is an automated comment for commit 93e8ecf with description of existing statuses. It's updated for the latest CI running
|
@@ -96,12 +96,17 @@ using namespace std::chrono_literals; | |||
static constexpr size_t log_peak_memory_usage_every = 1ULL << 30; | |||
|
|||
MemoryTracker total_memory_tracker(nullptr, VariableContext::Global); | |||
MemoryTracker background_memory_tracker(&total_memory_tracker, VariableContext::User, false); |
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.
I guess it worth a comment (I guess it fixes this #48774 issue)
Also why is this a problem in the first place? Maybe there is something incorrect in waiting of thread pools?
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 don't have an order when global objects are destroyed.
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.
Maybe I described it not good enough, I meant that if threads was joined, then background_memory_tracker
cannot be used anymore, so it should be enough to ensure that the threads was joined, or am I missing something?
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.
background_memory_tracker do some staff in d-tor. That might be the problem I think. It calls some object which could be destroyed already.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add MemoryTracker for the background tasks (merges and mutation). Introduces
merges_mutations_memory_usage_soft_limit
andmerges_mutations_memory_usage_to_ram_ratio
settings that represent the soft memory limit for merges and mutations. If this limit is reached ClickHouse won't schedule new merge or mutation tasks. AlsoMergesMutationsMemoryTracking
metric is introduced to allow observing current memory usage of background tasks.Resubmit #46089. Closes #48774.
Documentation entry for user-facing changes