Skip to content

Don't override remote, local throttling for merges, mutations and backups#81753

Merged
serxa merged 7 commits intomasterfrom
fix-throttling-override
Jun 27, 2025
Merged

Don't override remote, local throttling for merges, mutations and backups#81753
serxa merged 7 commits intomasterfrom
fix-throttling-override

Conversation

@serxa
Copy link
Copy Markdown
Member

@serxa serxa commented Jun 12, 2025

Local and remote throttling is now applied to merges, mutations, and backups.

A few interrelated changes are combined in this PR:

  1. Added interface IThrottler that hides throttling details.
  2. Added a possibility to combine multiple independent throttlers in one (see ThrottlerArray and addThrottler())
  3. As we could use multiple independent throttlers, the way we count metrics has changed. Instead of incrementing ProfileEvents in places where a throttler is used, we attach ProfileEvents directly to the root throttler. If multiple throttlers are used, all related ProfileEvents will be updated.
  4. Merges, mutations, and backups now use two independent throttlers, instead of one (see changelog entry). For example, merges that read from S3 are now accounted not only by max_merges_bandwidth_for_server throttler, but also by max_remote_read_network_bandwidth_for_server throttler.

Note that merges and mutations were accounted in remote/local throttler a long time ago, but after #57877 they obtained an independent throttler and were excluded from remote/local throttling (even if merge-specific or mutation-specific throttling was disabled). The same logic was applied for BACKUPs even earlier when backup throttling was introduced.

Changelog category (leave one):

  • Backward Incompatible Change

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

Previously, BACKUP queries, merges and mutations were not using server-wide throttlers for local (max_local_read_bandwidth_for_server and max_local_write_bandwidth_for_server) and remote (max_remote_read_network_bandwidth_for_server and max_remote_write_network_bandwidth_for_server) traffic, instead they were only throttled by dedicated server settings (max_backup_bandwidth_for_server, max_mutations_bandwidth_for_server and max_merges_bandwidth_for_server). Now, they use both types of throttlers simultaneously.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 12, 2025

Workflow [PR], commit [5c05632]

@clickhouse-gh clickhouse-gh bot added the pr-backward-incompatible Pull request with backwards incompatible changes label Jun 12, 2025
@serxa serxa requested review from azat, Copilot and vitlibar June 12, 2025 18:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors throttling across S3, file, and backup operations by introducing a unified throttling interface and combining multiple throttlers where needed, with built-in metrics reporting.

  • Introduce IThrottler and replace all ThrottlerPtr forward declarations.
  • Add ThrottlerArray and addThrottler() for composing multiple independent throttlers.
  • Update Throttler to record its own ProfileEvents and simplify call-sites to use the new interface.
  • Refactor various I/O classes (S3, Azure, local file) and backup worker to use the new throttling model.

Reviewed Changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Common/IThrottler.h New interface for throttlers.
src/Common/Throttler.h/.cpp Update Throttler to implement IThrottler and record events.
src/Common/ThrottlerArray.h Add ThrottlerArray and addThrottler().
src/IO/*.cpp, *.h Swap out Throttler_fwd.h for IThrottler.h and update calls.
src/IO/S3RequestSettings.cpp Attach ProfileEvents to S3 throttlers.
src/Backups/BackupsWorker.cpp Combine backup throttler with remote/local via addThrottler().
src/Disks/ObjectStorages/IObjectStorage.h Remove legacy per-storage throttler members.
Comments suppressed due to low confidence (2)

src/Common/ThrottlerArray.h:1

  • [nitpick] Consider adding unit tests for ThrottlerArray and addThrottler() to verify combined throttling behavior and that add() correctly sums sleep times and propagates to all underlying throttlers.
#pragma once

src/Common/IThrottler.h:1

  • [nitpick] Add a high-level comment or external documentation explaining the purpose of IThrottler, how to implement it, and examples for using ThrottlerArray and addThrottler().
#pragma once

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@azat azat self-assigned this Jun 12, 2025
@serxa
Copy link
Copy Markdown
Member Author

serxa commented Jun 13, 2025

Failed tests are related because I broke how ProfileEvents are counted for throttlers. So I will introduce a new set of ProfileEvents for query-level throttlers. I think it is beneficial on its own because it is essential to distinguish query-level throttling and server-level throttling.

I believe nobody wants to dig into high *ThrottlerSleepMicroseconds ProfileEvents to discover that server-level throttling is okay (the server is not overloaded), but there are occasional queries with very low query-level throttling settings like max_local_read_bandwidth.

@serxa serxa enabled auto-merge June 16, 2025 19:52
@serxa serxa disabled auto-merge June 19, 2025 14:15
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 25, 2025

Workflow [PR], commit [4800241]

Summary:

job_name test_name status info comment
Integration tests (asan, old analyzer, 4/6) failure
Job Timeout Expired FAIL

@serxa serxa removed the request for review from vitlibar June 27, 2025 15:25
@serxa serxa added this pull request to the merge queue Jun 27, 2025
Merged via the queue into master with commit 4c757b8 Jun 27, 2025
119 of 122 checks passed
@serxa serxa deleted the fix-throttling-override branch June 27, 2025 15:50
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backward-incompatible Pull request with backwards incompatible changes pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants