Take memory allocations from librdkafka threads into account#15740
Merged
alexey-milovidov merged 2 commits intoClickHouse:masterfrom Oct 8, 2020
Merged
Take memory allocations from librdkafka threads into account#15740alexey-milovidov merged 2 commits intoClickHouse:masterfrom
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
Conversation
Wrap them into StorageKafkaInterceptors to allow access to private fields and add logging inside interceptors if something fails. This is also preparation for ThreadStatus interceptor.
3fc4fbb to
1f51de3
Compare
Member
Author
01514_distributed_cancel_query_on_error - #15745 |
| // | ||
| /// And store them in a separate list (thread_statuses) to make sure that they will be destroyed, | ||
| /// regardless how librdkafka calls the hooks. | ||
| /// But this can trigger use-after-free if librdkafka will not destroy threads after rd_kafka_wait_destroyed() |
Member
There was a problem hiding this comment.
I will assume this will never happen...
alexey-milovidov
approved these changes
Oct 8, 2020
azat
added a commit
to azat/ClickHouse
that referenced
this pull request
Oct 23, 2020
This should significantly reduce the MemoryTracking drift, test shows that there is 0 drift after query storm (100 queries, via http/tcp/tcp in one session). TL;DR; To track memory, clickhouse creates memory tracker object for each thread **explicitly**, but until it is not created the memory allocations are not under account. There should not be lot of allocations w/o memory tracker, since most of the time it is created early enough, but even this maybe enough to trigger some problems. Plus sometimes it is not possible to create it, for example some 3d party library does not allow to do this explicitly: - for example before ClickHouse#15740 allocations from librdkafka threads, - or even worse, poco threads, they don't have any routines to do this. This won't be a problem for `MemoryTracking` metric if the deallocation will be done from the same thread w/o memory tracker (or vise versa), but this is not always true. NOTE, that this will slow down per-thread allocations w/o memory tracker, since before this patch there were no memory tracking for them while now they will be accounted in total_memory_tracker, and for total_memory_tracker max_untracked_memory is always reached. But this should not be significant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Take memory allocations from librdkafka threads into account
Details
HEAD: