[improvement](be) Add CPU-aware S3 rate limiter#65420
Open
0AyanamiRei wants to merge 59 commits into
Open
Conversation
Add three new CPU-aware S3 limiter configs: - s3_get_token_per_second_per_core: GET QPS per CPU core (0 to disable) - s3_put_token_per_second_per_core: PUT QPS per CPU core (0 to disable) - token_per_second_max: Hard cap on derived QPS (0 means no cap) When per-core values are > 0, effective QPS = per_core_value * CpuInfo::num_cores() When per-core values are = 0, existing absolute configs are used (preserving backward compatibility). All new configs default to 0, maintaining existing behavior. Affected files: - be/src/common/config.h: Added DECLARE statements - be/src/common/config.cpp: Added DEFINE statements with validators Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix naming inconsistency: all other S3 rate-limiter config options carry the s3_ prefix. This public API change must be fixed now before downstream code starts using these configs. - DECLARE_mInt64(s3_token_per_second_max) in config.h - DEFINE_mInt64(s3_token_per_second_max) in config.cpp - DEFINE_Validator(s3_token_per_second_max) in config.cpp Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add S3RateLimiterConfig struct and get_effective_s3_rate_limiter_config() to centralize the logic for computing effective rate limiter parameters from both legacy configs and new per-core configs. Changes: - Add S3RateLimiterConfig struct in s3_util.h - Add get_effective_s3_rate_limiter_config() declaration and implementation - Add helper functions checked_positive_product() and apply_s3_token_per_second_max() - Add required includes: <limits> and util/cpu_info.h This is Task 2 of the S3 rate limiter refactoring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n and dynamic reset
Replace raw config::s3_{get,put}_* accesses with get_effective_s3_rate_limiter_config()
in both S3ClientFactory constructor (initialization) and check_s3_rate_limiter_config_changed()
(dynamic reset). This ensures per-core QPS scaling and the s3_token_per_second_max cap are
applied consistently at startup and on every dynamic config check.
Also add an INFO log on successful limiter reset to make config changes observable in logs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cover four scenarios: absolute GET config, per-core GET without cap, per-core GET with cap, and per-core PUT with cap. Add S3RateLimiterConfigGuard RAII helper to save/restore all nine related configs after each test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace manual save/restore of 3 config values with S3RateLimiterConfigGuard that handles all 9 relevant S3 rate limiter configs. This prevents state contamination between tests. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…r-core and cap config
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
0AyanamiRei
changed the title
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CPU-aware S3 rate limiter calculation multiplied per-core QPS by BE CPU cores before applying s3_token_per_second_max. Extremely large mutable per-core settings could overflow int64 and trigger a CHECK even when a valid max cap was configured. This change computes the CPU-derived QPS with a wider intermediate value and applies the configured cap safely. It also adds unit coverage for the overflow-with-cap case and PUT limiter rebuild when the max cap changes.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=S3UTILTest.effective_s3_rate_limiter_config_caps_overflowing_per_core_get_config
- ./run-be-ut.sh --run --filter='S3UTILTest.*'
- Behavior changed: No
- Does this need documentation: No
0AyanamiRei
force-pushed
the
feature/cpu-aware-s3-limiter
branch
from
July 9, 2026 11:24
5287ce9 to
a4effbe
Compare
0AyanamiRei
requested review from
Gabriel39,
gavinchou,
liaoxin01 and
morningman
as code owners
July 10, 2026 07:20
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: The S3 bytes limiter ignored the result of add(), unlike the request QPS limiter, and the per-core bandwidth configs defaulted to disabled. Check the bytes limiter result consistently and set GET/PUT per-core defaults to 100 MiB/s. ### Release note Set default per-core S3 GET/PUT bandwidth limits to 100 MiB/s and consistently handle limiter failures. ### Check List (For Author) - Test: No need to test (per user request; format checks only) - Behavior changed: Yes (S3 bandwidth limiting defaults to 100 MiB/s per CPU core) - Does this need documentation: No
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Found one blocking issue in the S3 bandwidth limiter propagation.
Critical checkpoint conclusions:
- Goal/test coverage: The PR adds CPU-aware S3 request and byte limiters and BE unit tests for config calculation and dynamic rebuild. It does not fully apply the new byte limiter to every object-storage path that already shares the S3 rate-limiter contract.
- Scope/focus: The core PR scope is focused on S3 limiter config, S3 client enforcement, and tests.
- Concurrency/lifecycle: Limiter reset keeps the existing holder locking and atomic update guards; I did not find a separate concurrency issue in the authoritative PR file set.
- Config/compatibility: The new mutable BE configs are dynamically observed through the existing checker, but the shared Azure path is left incompatible with the new bandwidth limiter behavior.
- Parallel paths: Native S3 charges bytes for PUT/UploadPart/GetObject; Azure still charges only request count through the same shared limiter gate.
- Tests/CI: The added unit tests cover S3 effective config and rebuild behavior, but not the Azure parallel path. I could not run BE UTs in this runner because the checkout lacks thirdparty/installed and protoc. git diff --check was clean for the GitHub PR files; check-format.sh could not run because clang-format 16 is unavailable.
User focus: no additional user-provided review focus was supplied.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65420 Problem Summary: The CPU-aware S3 bandwidth limiter was applied only to the native S3 client. Azure shared the request limiter but did not charge bytes for uploads or downloads, so enabling the limiter produced inconsistent behavior. Centralize request and byte limiter acquisition and pass payload sizes from Azure put, multipart upload, and get operations. ### Release note Apply object-storage bandwidth limits consistently to native S3 and Azure Blob Storage. ### Check List (For Author) - Test: Unit Test - `S3UTILTest.*` - Behavior changed: Yes. Azure Blob Storage data transfers now consume the shared byte-rate limiter when S3 rate limiting is enabled. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
|
/review |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Automated review completed for PR #65420.
I did not find a new blocking issue to submit as an inline comment. The earlier Azure byte-accounting gap is already covered by the existing inline thread discussion_r3557395816, and the current head now routes Azure put_object, upload_part, and get_object payload sizes through the shared acquire_s3_rate_limit path.
Critical checkpoint conclusions:
- Goal/test proof: the PR adds CPU-aware S3 request limits plus byte-rate limiting and centralizes effective limiter calculation. The added BE unit tests cover absolute/per-core/cap/dynamic-reset cases; I could not complete a local BE UT run in this checkout.
- Scope: the BE changes are focused around S3/Azure object-storage limiter config, helper propagation, and tests.
- Concurrency/lifecycle: limiter reset continues to use
TokenBucketRateLimiterHolderlocks; startup use ofCpuInfo::num_cores()is after BECpuInfo::init(), and the BE test harness also initializesCpuInfo. - Config/dynamic behavior: new mutable configs have non-negative validators, and
check_s3_rate_limiter_config_changed()observes request and byte limiter changes. - Compatibility/parallel paths: native S3 and Azure transfer paths now both use the shared request+byte limiter helper. Cloud recycler has its own config surface and was not changed by this BE-scoped PR.
- Tests/style:
git diff --checkis clean. I attempted./run-be-ut.sh --run --filter='S3UTILTest.*', but interrupted it while it was still bootstrapping dependencies/submodules before reaching test execution. - User focus: no additional user-provided focus points.
Subagent conclusions:
optimizer-rewrite: no optimizer/Nereids surface and no indirect semantic issue found.tests-session-config: no new tests/config/CI candidate found; existing Azure byte-accounting concern is duplicate-covered and fixed in head.- Convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same final ledger/comment set.
Contributor
TPC-H: Total hot run time: 29332 ms |
Contributor
TPC-DS: Total hot run time: 180020 ms |
Contributor
TPC-DS: Total hot run time: 177199 ms |
Contributor
ClickBench: Total hot run time: 24.67 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: Azure per-retry byte accounting requires interpreting Azure HTTP request bodies and range headers. Keeping that SDK-specific policy and parsing in the generic S3 utility made the abstraction unnatural. Move the Azure request-byte extraction, context key, and per-retry policy into the Azure object-storage client module while preserving the existing retry-attempt accounting behavior. ### Release note None ### Check List (For Author) - Test: Unit Test - ./build.sh --be -j48 - ./run-be-ut.sh --run --filter='S3ObjStorageClientMockTest.*:AzureObjStorageClientRateLimiterTest.*:S3UTILTest.*' -j48 - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
|
/review |
…tion ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65420 Problem Summary: Object-store SDK retry hooks treated a bytes rate-limiter rejection as an invariant violation and aborted the BE. Log the rejection as a warning and continue the Native S3 or Azure request instead. Clarify that S3 client reset must compare complete effective configurations because distinct configurations can have the same hash. Also document why configuration validators must validate their own candidate before publication, avoiding stale or concurrently published values. ### Release note Object-store requests no longer abort the BE when a bytes rate limiter rejects an SDK attempt; the rejection is logged as a warning and the request continues. ### Check List (For Author) - Test: Unit Test - `./run-be-ut.sh --run --filter=S3ClientFactoryTest.ObjClientHolderResetDistinguishesHashCollisions:S3ObjStorageClientMockTest.*:AzureObjStorageClientRateLimiterTest.*:ConfigConcurrencyTest.*:ConfigTest.*:S3UTILTest.* -j48` - `./build.sh --be -j48` - `build-support/check-format.sh` - Behavior changed: Yes. Bytes limiter rejection now warns and allows the SDK request to continue instead of aborting the BE. - Does this need documentation: No
sollhui
reviewed
Jul 16, 2026
sollhui
reviewed
Jul 16, 2026
| { | ||
| std::unique_lock write {rate_limiter_rw_lock}; | ||
| rate_limiter = std::make_unique<TokenBucketRateLimiter>(max_speed, max_burst, limit); | ||
| rate_limiter = std::move(new_rate_limiter); |
Contributor
There was a problem hiding this comment.
[P1] Preserve outstanding rate/debt across reset. add_with_config() snapshots the old limiter and releases the holder lock before limiter->add() sleeps, while reset() immediately publishes a fresh full bucket. If the rate is lowered during an old-generation sleep, retired requests still wake on the old schedule while new requests consume the new bucket, so traffic can exceed the newly configured cap. Please carry debt/reservations into the new generation or gate new admissions until retired waiters drain, and add a barrier-based reset-with-waiters test.
Contributor
Author
There was a problem hiding this comment.
maybe we dont need to care this problem？
Contributor
There was a problem hiding this comment.
Automated review completed for 97e1688bbf5572e60e15508aeb4eb76d3c92d1f1 against a6745ebc5cbcaa2e2304653494211bf15747cee8.
I found one remaining P1 in the Azure retry path: byte admission can sleep after x-ms-date is created and before Shared Key signing, so a valid low rate can make every attempt expire before transmission. The inline comment explains the reachable interleaving and fix direction. The current AWS-only sibling is already covered by discussion_r3596869021, and the outstanding limiter reset/debt issue is already covered by discussion_r3596869274 / discussion_r3595014308; I did not duplicate those threads.
Critical checkpoint conclusions:
- Goal and scope: The PR implements CPU-aware GET/PUT QPS and byte limiting, cloud-internal classification, dynamic cloud refresh, retry-attempt byte accounting, exact client-cache keys, and candidate-before-publication config validation. The changes are generally focused and extensively unit-tested, but the Azure authentication ordering means the bandwidth-limiter goal is not yet correct end to end.
- Concurrency: Config mutation/publication is serialized and the checked callbacks do not create a concrete lock cycle. Holder/factory state uses the expected locks and shared ownership. The remaining old-generation debt problem is already reported in the live threads; the separate disabled fast-path read is a linearizable in-flight admission, not a new defect.
- Lifecycle and configuration:
CpuInfois initialized before factory construction, cloud refresh compares independent QPS/byte snapshots, and-1/0/max compatibility is symmetric. The existing classic-BE refresh concern is already covered bydiscussion_r3592467477. - Parallel paths and conditions: Internal/external bucket classification survives construction, refresh, reset, and full-key cache lookup. AWS and Azure charge once per SDK attempt with the expected body/range shapes, but Azure's service/client policy order creates the reported timestamp-expiry failure. Production byte limiters have no hard rejection cap, so the log-and-continue rejection branch is not otherwise reachable.
- Tests and results: Added BE tests cover effective-limit arithmetic, compatibility values, refresh, config concurrency, classification/cache behavior, disabled fast paths, and retry charging. They do not exercise Azure's real policy order across a long admission delay, and the existing reset/debt thread still needs a barrier-based test. No result files are involved.
- Error handling, observability, and performance: Existing logs and bvars are adequate, disabled-holder fast paths avoid the prior no-op synchronization, and no additional silent error, allocation, or hot-path issue was substantiated.
- Compatibility and persistence: No storage format, wire protocol, FE/BE transmission, transaction, data-write, or EditLog behavior changes are involved.
User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
Validation was static only, as required by the review runner. I did not build or run tests. The current large-file, formatter, license, and review-status checks pass; compile and unit-test checks were still pending when reviewed.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 30041 ms |
Contributor
TPC-DS: Total hot run time: 178547 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: S3 bytes throttling used RequestSignedHandler, so a long limiter wait could leave the already-signed request with a stale SigV4 timestamp. Charge the first attempt after QPS admission and immediately before entering the SDK, then use RequestRetryHandler to charge retry attempts before the SDK signs the next request. Replace the manually invoked callback test with a real AWS SDK retry loop. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=S3ObjStorageClientMockTest.* -j48 - ./build.sh --be -j48 - Behavior changed: Yes. S3 byte throttling now occurs before SigV4 signing for every SDK attempt. - Does this need documentation: No
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29375 ms |
Contributor
TPC-DS: Total hot run time: 177484 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: The PR had expanded into a generic runtime config validator redesign and AWS SDK retry-boundary throttling. Keep this PR scoped to the CPU-aware object-storage limiter by restoring the existing config framework and signed-handler behavior, documenting the runtime validator limitation for a separate PR, and keeping limiter tests independent of generic set_config validation. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=ConfigTest.*:S3UTILTest.*:S3ObjStorageClientMockTest.* -j8 - ./build.sh --be -j64 - ./build-support/check-format.sh - Behavior changed: Yes. Revert the out-of-scope config framework and retry-boundary changes. - Does this need documentation: No
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: The previous rollback removed the AWS pre-sign retry hook but left per-attempt byte accounting in both Native S3 and Azure. Restore the pre-retry design so each Doris logical GET or PUT request acquires QPS and bytes once before entering the SDK. Remove AWS signed handlers, Azure per-retry policies, SDK request parsing, split limiter acquisition APIs, and per-attempt tests. Add focused tests for the logical-request boundary. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=S3ObjStorageClientMockTest.*:AzureObjStorageClientRateLimiterTest.*:S3UTILTest.* -j64 - ./build.sh --be -j64 - ./build-support/check-format.sh - Behavior changed: Yes. Object-storage bytes are charged once per Doris logical request instead of once per SDK retry attempt. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29282 ms |
Contributor
TPC-DS: Total hot run time: 176978 ms |
Contributor
ClickBench: Total hot run time: 24.85 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
The existing S3 GET/PUT token bucket rate limiter uses fixed per-BE QPS settings. This is not suitable for heterogeneous deployments because a BE with many CPU cores and a BE with few CPU cores receive the same S3 request budget.
This PR adds CPU-aware S3 rate limiter settings for GET and PUT requests. When the new per-core settings are left as 0, Doris keeps the existing absolute limiter behavior. When a per-core setting is positive, Doris derives the effective per-BE S3 QPS from BE CPU cores and caps it with s3_token_per_second_max when configured.
The PR also centralizes effective limiter config calculation, reuses it for initialization and dynamic rebuild, and guards CPU-derived QPS calculation from overflow before applying the max cap.
Release note
Add BE configs to support CPU-aware S3 GET/PUT request rate limiting.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)