Don't override remote, local throttling for merges, mutations and backups#81753
Don't override remote, local throttling for merges, mutations and backups#81753
Conversation
There was a problem hiding this comment.
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
IThrottlerand replace allThrottlerPtrforward declarations. - Add
ThrottlerArrayandaddThrottler()for composing multiple independent throttlers. - Update
Throttlerto record its ownProfileEventsand 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
ThrottlerArrayandaddThrottler()to verify combined throttling behavior and thatadd()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 usingThrottlerArrayandaddThrottler().
#pragma once
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
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 |
Local and remote throttling is now applied to merges, mutations, and backups.
A few interrelated changes are combined in this PR:
max_merges_bandwidth_for_serverthrottler, but also bymax_remote_read_network_bandwidth_for_serverthrottler.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):
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_serverandmax_local_write_bandwidth_for_server) and remote (max_remote_read_network_bandwidth_for_serverandmax_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_serverandmax_merges_bandwidth_for_server). Now, they use both types of throttlers simultaneously.Documentation entry for user-facing changes