Skip to content

Take memory allocations from librdkafka threads into account#15740

Merged
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
azat:kafka-memory-accounting
Oct 8, 2020
Merged

Take memory allocations from librdkafka threads into account#15740
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
azat:kafka-memory-accounting

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Oct 7, 2020

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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:

  • 3fc4fbbd5caee511b319e6bbf8e6cdb72f62f0f3

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.
@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 7, 2020
@azat azat force-pushed the kafka-memory-accounting branch from 3fc4fbb to 1f51de3 Compare October 7, 2020 20:18
@azat
Copy link
Copy Markdown
Member Author

azat commented Oct 7, 2020

Functional stateless tests (release, DatabaseOrdinary) — fail: 1, passed: 2315, skipped: 4

01514_distributed_cancel_query_on_error - #15745

@alexey-milovidov alexey-milovidov self-assigned this Oct 8, 2020
//
/// 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()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will assume this will never happen...

@alexey-milovidov alexey-milovidov merged commit c0cb18b into ClickHouse:master Oct 8, 2020
@azat azat deleted the kafka-memory-accounting branch October 8, 2020 21:20
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants