Skip to content

Use background thread pool for distributed sends#10263

Merged
alexey-milovidov merged 4 commits intoClickHouse:masterfrom
azat:distributed-send-bg-pool
Apr 19, 2020
Merged

Use background thread pool for distributed sends#10263
alexey-milovidov merged 4 commits intoClickHouse:masterfrom
azat:distributed-send-bg-pool

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Apr 14, 2020 •
edited
Loading

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Use background thread pool (background_schedule_pool_size) for distributed sends

Detailed description / Documentation draft:
After #8756 the problem with background threads for distributed sends became even worse (since thread per volume will be created).

Fixes: #9551
Refs: #8756

See-also: #10315 (same thing for Buffer engine)

azat
azat commented Apr 14, 2020
View reviewed changes
azat
azat commented Apr 14, 2020
View reviewed changes
@blinkov blinkov added the pr-improvement Pull request with some product improvements label Apr 14, 2020
@azat azat force-pushed the distributed-send-bg-pool branch 2 times, most recently from 33b97f7 to a40b4a1 Compare April 14, 2020 18:37
@alexey-milovidov alexey-milovidov self-requested a review April 14, 2020 18:57
@azat azat force-pushed the distributed-send-bg-pool branch from a40b4a1 to 19ab04b Compare April 14, 2020 20:38
@azat
Copy link
Copy Markdown
Member Author

azat commented Apr 15, 2020 •
edited
Loading

test_settings_constraints_distributed/test.py::test_insert_clamps_settings
test_dictionaries_mysql/test.py::test_load_mysql_dictionaries

Fails in upstream/master too

test_settings_constraints_distributed can be fixed with SYSTEM FLUSH DISTRIBUTED

@azat azat force-pushed the distributed-send-bg-pool branch from 19ab04b to c78803b Compare April 15, 2020 07:00
@azat
Copy link
Copy Markdown
Member Author

azat commented Apr 15, 2020

test_settings_constraints_distributed can be fixed with SYSTEM FLUSH DISTRIBUTED

Added

@azat azat force-pushed the distributed-send-bg-pool branch from c78803b to cb818d7 Compare April 15, 2020 08:12
azat
azat commented Apr 15, 2020
View reviewed changes
@azat azat force-pushed the distributed-send-bg-pool branch 2 times, most recently from da37e24 to 5c61f0f Compare April 16, 2020 17:15
@azat azat mentioned this pull request Apr 16, 2020
Merged
@azat
Copy link
Copy Markdown
Member Author

azat commented Apr 18, 2020

PVS check — Found 1 new errors, total 20 errors

Does not looks related

test_insert_into_distributed/test.py::test_inserts_batching

Uses sleep over system flush distributed, and I decided do just increase the sleep time (maybe it will handle some corner cases, even though I tried to cover all of them)

@azat azat force-pushed the distributed-send-bg-pool branch from ba91491 to 633f4d6 Compare April 18, 2020 07:37
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Apr 18, 2020

test_inserts_batching did not fix.

@azat azat force-pushed the distributed-send-bg-pool branch from 633f4d6 to 5054322 Compare April 18, 2020 17:28
@azat azat force-pushed the distributed-send-bg-pool branch from 5054322 to fe4be16 Compare April 18, 2020 21:47
azat added 2 commits April 19, 2020 11:20
@azat
Update comment for background_schedule_pool_size
201d5d5 
Include info about:
- kafka streaming
- dns cache updates
@azat
Use thread pool (background_distributed_schedule_pool_size) for distr…
5d11118 
…ibuted sends

After ClickHouse#8756 the problem with 1 thread for each (distributed table, disk)
for distributed sends became even worse (since there can be multiple
disks), so use predefined thread pool for this tasks, that can be
controlled with background_distributed_schedule_pool_size knob.
@azat azat force-pushed the distributed-send-bg-pool branch from fe4be16 to 5d11118 Compare April 19, 2020 09:12
@alexey-milovidov alexey-milovidov merged commit 61d33a8 into ClickHouse:master Apr 19, 2020
@azat
Copy link
Copy Markdown
Member Author

azat commented Apr 19, 2020

Actually not sure that "pr-improvement" will be enough, since it can be pretty tricky to debug the problems with distributed sends i.e. why it became slower (for "regular" user), maybe backward incompatible is better?

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Apr 19, 2020

It's only relevant when using a huge number of Distributed tables (rare case). And it should not become slower as we have 16 background threads. Data is sent almost as is without any processing on our side, so it will either saturate the network or there are slow peers (that will require additional debugging).

@azat azat deleted the distributed-send-bg-pool branch April 20, 2020 07:51
azat added a commit to azat/ClickHouse that referenced this pull request Apr 22, 2020
@azat
Add tasks/memory metrics for distributed/buffer schedule pools
d854049 
Follow-up-for: ClickHouse#10315
Follow-up-for: ClickHouse#10263
This was referenced Apr 22, 2020
Merged
Merged
alesapin pushed a commit that referenced this pull request Apr 26, 2020
@azat @alesapin
Add tasks/memory metrics for distributed/buffer schedule pools
6dc9908 
Follow-up-for: #10315
Follow-up-for: #10263
@azat azat mentioned this pull request Apr 27, 2020
Closed
@azat azat mentioned this pull request Aug 26, 2020
Merged
azat added a commit to azat/ClickHouse that referenced this pull request Aug 26, 2020
@azat
Fix DistributedFilesToInsert metric (zeroed when it should not)
a588947 
CurrentMetrics::Increment add amount for specified metric only for the
lifetime of the object, but this is not the intention, since
DistributedFilesToInsert is a gauge and after ClickHouse#10263 it can exit from
the callback (and enter again later, for example after SYSTEM STOP
DISTRIBUTED SEND it will always exit from it, until SYSTEM START
DISTRIBUTED SEND).

So make Increment member of a class (this will also fix possible issues
with substructing value on DROP TABLE).
@alexey-milovidov alexey-milovidov mentioned this pull request Dec 31, 2020
Closed
@alexey-milovidov alexey-milovidov mentioned this pull request Jun 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov Awaiting requested review from alexey-milovidov

Assignees

No one assigned

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Lower number of threads for background sends in Distributed tables

3 participants

@azat @alexey-milovidov @blinkov