Socket level throttling for S3 read and write requests by serxa · Pull Request #81837 · ClickHouse/ClickHouse

serxa · 2025-06-13T19:09:25Z

Depends on #81753 and should be reviewed after it is merged

Follows similar logic as #65182 and resolves the same problem: Coarse-grained throttling based on S3 request (or response) size does not work well when request sizes are larger than the token bucket size (usually 1 sec * max_speed B/sec). Throttling such large chunks leads to underutilization of real network bandwidth and idle-waiting of threads on throttlers for multiple seconds. Throttling individual socket operations instead of whole S3 requests solves the problem by distributing traffic consumption over a longer duration. This allows effective replenishment and use of the token bucket volume.

Changelog category (leave one):

Improvement

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

S3 read and write requests are throttled on the HTTP socket level (instead of whole S3 requests) to avoid issues with max_remote_read_network_bandwidth_for_server and max_remote_write_network_bandwidth_for_server throttling.

… backups

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

clickhouse-gh · 2025-06-13T19:09:51Z

Workflow [PR], commit [29d7544]

clickhouse-gh · 2025-06-25T10:49:11Z

Workflow [PR], commit [e01b689]

Summary: ❌

job_name	test_name	status
Stateless tests (amd_asan, distributed plan, sequential)		failure
	00002_log_and_exception_messages_formatting	FAIL
Stateless tests (amd_ubsan, parallel)		failure
	01825_new_type_json_ghdata_insert_select	FAIL

CheSema · 2025-06-30T14:15:28Z

We spoke about that.
The outcome is:

try to sleep before consumption
make sure that consumption is more granular for reading, we expect read no more that several kilobytes in one call
think how is it possible to not sleep too long (longer that read/write timeout) and not sleep after query deadline

serxa · 2025-07-01T18:59:29Z

Addressing review:

try to sleep before consumption

I've moved throttling to places before syscalls.
Added budget because we don't know how much resource will be needed before syscall
Added minimum amount to request through throttler (32KB) to avoid too frequent calls, because the current implementation has a mutex. Rough benchmarks show that 32KB should be enough for 40Gbps network throttler to never be a bottleneck itself (details)

make sure that consumption is more granular for reading, we expect read no more that several kilobytes in one call

think how is it possible to not sleep too long (longer that read/write timeout) and not sleep after query deadline

Not addressed yet

serxa · 2025-07-04T16:05:51Z

think how is it possible to not sleep too long (longer that read/write timeout) and not sleep after query deadline

I've added support for limits inIThrottler::throttle() and passed half of socket recv/send timeouts there. I think this should be good to avoid any timeout due to throttling.
Regarding query deadlines I think it should be handled in a separate PR by adding cancellation points into all blocking calls, including sleepForNanoseconds() which is used by throttlers. Now it is prepared for signals but just reenters sleep, so this is a good point for cancellation exception to be thrown. To support it I made DB::Throttler code exception-safe in where it calls sleep.

CheSema

LGTM.
But tests are broken

serxa · 2025-07-18T15:02:47Z

I have found the problem with tests. It's an unsigned integer overflow that happens if max_block_us == size_t(-1)

❯ utils/c++expr -I -i utility -i base/types.h 'static constexpr auto NS = 1000000000UL; double tokens_value = -48576.0; size_t max_block_us = 18446744073709551615uz; size_t max_speed_value = 1000000; Int64 block_ns = static_cast<Int64>(-tokens_value / max_speed_value * NS); OUT(std::min<Int64>(max_block_us * 1000, block_ns)) OUT(block_ns)'
std::min<Int64>(max_block_us * 1000, block_ns) -> -1000 <====== negative value passed forward to sleep function taking UInt64
block_ns -> 48576000

UPD. Interestingly enough, Copilot was able to find it as well. I'd better ask it before I figure it out by myself...

Copilot

Pull Request Overview

This PR implements socket-level throttling for S3 read and write requests to address issues with coarse-grained throttling when request sizes exceed token bucket capacity. The change moves throttling from the S3 request level to individual socket operations, enabling better distribution of traffic consumption and more effective use of token bucket volumes.

Refactors throttler interface from add() to throttle() method with improved blocking semantics
Implements socket-level throttling in Poco networking stack with budget-based approach
Integrates throttlers into HTTP connection pool and S3 operations via thread-local scopes

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/Common/Throttler.h	Updates throttler interface to use `throttle()` method instead of `add()`
src/Common/Throttler.cpp	Implements new throttling logic with blocking semantics and thread safety
src/IO/WriteBufferFromS3.cpp	Replaces request-level throttling with socket-level throttling scopes
src/IO/ReadBufferFromS3.cpp	Removes manual throttling calls, delegates to socket-level implementation
base/poco/Net/src/SocketImpl.cpp	Adds socket-level throttling with budget management
src/Common/CurrentThread.h	Adds throttling scope classes for read/write operations
src/Common/HTTPConnectionPool.cpp	Integrates throttlers into HTTP session management
Multiple buffer files	Updates throttler method calls from `add()` to `throttle()`

src/Common/Throttler.h

base/poco/Net/src/SocketImpl.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…House into socket-level-throttling

serxa · 2025-07-23T19:35:43Z

With the help of integration tests debugging I was able to find and fix the rest of the issues with tests:

The connection pool uses HTTPSessionClient::assign() to hijack the socket. Now, the throttlers are properly set afterwards.
The SecureSocketImpl does not use direct recv or send calls but instead uses SSL_read and SSL_write. Now these calls are wrapped with the throttler is exactly the same way as for SocketImpl.

serxa · 2025-07-25T10:38:55Z

PR / Integration tests (release, 3/4) (pull_request)Failing after 122m

This is a bit strange. I checked all the *.log files, and they are all successful and fast, but there is an error in the job logs. @tavplubix PTAL

INFO:root:All tests from group parallel7 passed
INFO:root:Tests from group parallel7 stats, ERROR count 0
INFO:root:Totally have 0 with status ERROR
INFO:root:Tests from group parallel7 stats, PASSED count 71
INFO:root:Totally have 940 with status PASSED
INFO:root:Tests from group parallel7 stats, FAILED count 0
INFO:root:Totally have 0 with status FAILED
INFO:root:Tests from group parallel7 stats, SKIPPED count 0
INFO:root:Totally have 0 with status SKIPPED
INFO:root:Tests from group parallel7 stats, BROKEN count 0
INFO:root:Totally have 0 with status BROKEN
INFO:root:Tests from group parallel7 stats, NOT_FAILED count 0
INFO:root:Totally have 0 with status NOT_FAILED
INFO:root:Totally finished tests 940/940
INFO:root:Overall success!
ERROR:root:Job killed by external timeout signal - setting status to failure!
INFO:root:Tests finished
INFO:root:Dumping dmesg
[Fri Jul 25 00:18:47 2025] br-38e14867abd6: port 1(veth4e56134) entered forwarding state
[Fri Jul 25 00:18:58 2025] br-38e14867abd6: port 1(veth4e56134) entered disabled state
[Fri Jul 25 00:18:58 2025] veth833236c: renamed from eth0
[Fri Jul 25 00:18:58 2025] br-38e14867abd6: port 1(veth4e56134) entered disabled state
[Fri Jul 25 00:18:58 2025] veth4e56134 (unregistering): left allmulticast mode
[Fri Jul 25 00:18:58 2025] veth4e56134 (unregistering): left promiscuous mode
[Fri Jul 25 00:18:58 2025] br-38e14867abd6: port 1(veth4e56134) entered disabled state
[Fri Jul 25 00:22:31 2025] br-d52f149df01e: port 5(veth0a42026) entered disabled state
[Fri Jul 25 00:22:31 2025] veth5573740: renamed from eth0
[Fri Jul 25 00:22:31 2025] br-d52f149df01e: port 4(veth8afaa0f) entered disabled state
[Fri Jul 25 00:22:31 2025] veth8e918f4: renamed from eth0
[Fri Jul 25 00:22:31 2025] br-d52f149df01e: port 5(veth0a42026) entered disabled state
[Fri Jul 25 00:22:31 2025] veth0a42026 (unregistering): left allmulticast mode
[Fri Jul 25 00:22:31 2025] veth0a42026 (unregistering): left promiscuous mode

serxa · 2025-07-28T22:57:45Z

All failed tests are unrelated

PR / Stateless tests (amd_asan, distributed plan, sequential) (pull_request)Failing after 74m

00002_log_and_exception_messages_formatting #84538
01825_new_type_json_ghdata_insert_select #84577

* [GLUTEN-1632][CH]Daily Update Clickhouse Version (20250729) * Fix build due to ClickHouse/ClickHouse#76802 * Fix build due to ClickHouse/ClickHouse#81837 * Fix build due to ClickHouse/ClickHouse#84011 * Fix gtest due to ClickHouse/ClickHouse#83599 --------- Co-authored-by: kyligence-git <gluten@kyligence.io> Co-authored-by: Chang chen <chenchang@apache.com>

serxa and others added 6 commits June 13, 2025 13:34

Do not override remote and local throttling for merges, mutations and…

e878318

… backups

fix typo

a5d88d3

Update src/Common/Throttler.cpp

a3f57af

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

independent ProfileEvents for query-level throttling

3f442d1

support socket-level throtting with IThrottler instances

50d0fbb

S3 read and write use socket-level throttling

29d7544

clickhouse-gh bot added the pr-improvement Pull request with some product improvements label Jun 13, 2025

serxa assigned CheSema Jun 13, 2025

serxa requested a review from Copilot June 13, 2025 21:26

This comment was marked as outdated.

Sign in to view

serxa force-pushed the socket-level-throttling branch from 05d76a5 to 29d7544 Compare June 25, 2025 11:03

serxa and others added 4 commits June 25, 2025 11:05

Merge branch 'master' into socket-level-throttling

5f59854

resolve conflict

f8333a7

Merge branch 'master' into socket-level-throttling

08a57ef

Merge branch 'master' into socket-level-throttling

563e3b4

throttle before socket operations and add budget

687ae2e

serxa added 4 commits July 4, 2025 12:18

Merge branch 'master' into socket-level-throttling

e2adf37

support max blocking time in throttlers

464a96b

socket-level throttling respects socket timeouts

ad5eff9

exception-safe blocking in throttler

aca057b

fix typo

68652d9

CheSema approved these changes Jul 17, 2025

View reviewed changes

serxa added 2 commits July 17, 2025 18:14

Merge branch 'master' into socket-level-throttling

c1de081

fix build

ad8a38f

serxa requested a review from Copilot July 18, 2025 15:00

This comment was marked as outdated.

Sign in to view

fix overflow and improve IThrottler interface

32fdab1

serxa requested a review from Copilot July 18, 2025 15:30

Copilot AI reviewed Jul 18, 2025

View reviewed changes

serxa and others added 7 commits July 19, 2025 00:13

Update src/Common/Throttler.h

27412b0

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix another overflow

12106cb

Merge branch 'socket-level-throttling' of github.com:ClickHouse/Click…

c86069f

…House into socket-level-throttling

use safer code

a874436

Merge branch 'master' into socket-level-throttling

551da82

fix build

47beb97

fix secure sockets throttling and fix connection pool integration

5a740d5

serxa added 3 commits July 24, 2025 16:35

fix tests by setting a proper timeouts

24de03e

typo

8ec623f

Merge branch 'master' into socket-level-throttling

38e92bc

Merge branch 'master' into socket-level-throttling

e01b689

serxa added this pull request to the merge queue Jul 28, 2025

Merged via the queue into master with commit e8935e4 Jul 28, 2025
123 of 125 checks passed

serxa deleted the socket-level-throttling branch July 28, 2025 23:21

robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 28, 2025

baibaichen pushed a commit to Kyligence/gluten that referenced this pull request Jul 29, 2025

Fix build due to ClickHouse/ClickHouse#81837

474e177

baibaichen pushed a commit to Kyligence/gluten that referenced this pull request Jul 29, 2025

Fix build due to ClickHouse/ClickHouse#81837

723072c

Conversation

serxa commented Jun 13, 2025

Changelog category (leave one):

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

Uh oh!

clickhouse-gh bot commented Jun 13, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

clickhouse-gh bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CheSema commented Jun 30, 2025

Uh oh!

serxa commented Jul 1, 2025

Uh oh!

serxa commented Jul 4, 2025

Uh oh!

CheSema left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

serxa commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serxa commented Jul 23, 2025

Uh oh!

serxa commented Jul 25, 2025

Uh oh!

serxa commented Jul 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

clickhouse-gh bot commented Jun 25, 2025 •

edited

Loading

serxa commented Jul 18, 2025 •

edited

Loading