[improvement](be) Add CPU-aware S3 rate limiter - #65420
Merged
liaoxin01 merged 26 commits intoJul 31, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
0AyanamiRei
changed the title
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
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.
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Jul 10, 2026
### 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
ClickBench: Total hot run time: 24.93 s |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29706 ms |
Contributor
TPC-DS: Total hot run time: 180343 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
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: 29978 ms |
Contributor
TPC-DS: Total hot run time: 179712 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: Merging the latest master into the S3 rate limiter branch kept both the old and new definitions of trino_connector_plugin_dir in BE config.cpp. Clang rejected the duplicate config variable and registration object definitions. Remove the obsolete plugins/connectors definition and keep the master definition that matches the FE plugins/trino_plugins default. ### Release note None ### Check List (For Author) - Test: No need to test (single merge-conflict cleanup; local testing was explicitly not requested) - Behavior changed: No - 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: 29538 ms |
Contributor
TPC-DS: Total hot run time: 178523 ms |
Contributor
ClickBench: Total hot run time: 24.86 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: The BE UT exposed that dynamic config validators checked the active value before the converted candidate was assigned. An invalid S3 limiter update such as s3_get_qps_per_core=-2 therefore passed validation and changed the live config. Publish the candidate before invoking the registered validator; the existing failure path restores the previous value. ### Release note Invalid dynamic BE configuration values are rejected without changing the active value. ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=S3RateLimiterConfigTest.*:ConfigValidatorTest.* -j48 - Behavior changed: Yes. Dynamic validators now evaluate the candidate value and roll back invalid updates. - 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 S3 limiter follow-up added redundant Azure timing scopes and immediately invoked lambdas. It also restricted new limiter configs to sentinel values even though the resolver already treats any negative or non-positive value as fallback, disabled, uncapped, or auto-detected. Remove the redundant Azure wrappers, align the config declarations and comments with the resolver semantics, restore the existing UPDATE_FIELD behavior, and remove the invalid dynamic-value unit test. ### Release note None ### Check List (For Author) - Test: No test run per request; clang-format and check-format passed - Behavior changed: Yes; negative limiter tuning values now follow the existing resolver semantics - 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: 29398 ms |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: The CPU-aware request-rate configs used the QPS abbreviation while the bandwidth configs spell out their units. Rename the four GET/PUT per-core and maximum request-rate configs to use requests_per_second consistently, and update all source, unit-test, and regression-test references. ### Release note Rename the new S3 CPU-aware request-rate configs to s3_get_requests_per_second_per_core, s3_put_requests_per_second_per_core, s3_get_requests_per_second_max, and s3_put_requests_per_second_max. ### Check List (For Author) - Test: No test run per request; clang-format and check-format passed - Behavior changed: Yes; the four new request-rate configuration names changed - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
|
/review |
Contributor
TPC-DS: Total hot run time: 177594 ms |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
There was a problem hiding this comment.
Automated review of beaa8ea2bf6e5f300be4ced32505ceb9eb33278c.
I found no additional distinct issues. Existing review threads already cover the limiter-generation, refill/refund, oversized-I/O accounting, logical-operation boundary, provider-retry, CPU-sizing, and ConfigTest snapshot concerns, so I did not duplicate them.
Critical checkpoint conclusions:
- Goal and proof: the manager/decorator design implements CPU-derived GET/PUT QPS and byte limits, internal-only cloud decoration, non-cloud compatibility, dynamic daemon refresh, cache collision safety, and short-read settlement. Unit, cloud regression, and Docker cgroup tests cover the main intended paths; no tests or builds were run in this review-only runner.
- Scope and parallel paths: the change is focused on object-storage admission plus directly related config/cache/test plumbing. S3 and Azure use the same decorator, every
ObjStorageClientmethod was traced, and presigning correctly remains local. - Concurrency and lifecycle: daemon shutdown joins through the existing latch; refresh is serialized; bucket mutation is spinlock-protected; holder generation replacement is shared-mutex-protected; charged generations are pinned for settlement. No new concern was found beyond the existing reset/refill/sizing threads.
- Configuration and compatibility: legacy absolute QPS settings remain the
-1fallback, zero disables per-core QPS, caps saturate safely, cgroup/override changes converge through the common daemon in cloud and non-cloud modes, and no FE/BE wire or storage format changes are introduced. - Error handling and writes: local admission rejection returns non-OK
EXCEEDED_LIMITwith HTTP 0, so readers do not enter provider-429 retry; writer paths propagate failure before performing the rejected provider call. No new transaction, persistence, crash-leak, or data-visibility issue was found. - Performance and observability: disabled buckets have lock-free fast-path checks, quiet refreshes preserve state, and separate GET/PUT QPS/byte sleep and rejection bvars plus reset/long-wait logs cover the new behavior. Remaining logical-call versus provider-request tradeoffs are already discussed.
- Test quality: result arithmetic, production reachability, and global config restoration were checked. The apparent second
full_conf_mapleak depends on the already-knownConfigTestteardown defect and is therefore not a distinct reportable issue. - Documentation: the release note describes the user-visible behavior, but the PR body should be refreshed to match the current config name
s3_rate_limiter_cpu_cores_overrideand HTTP-0 local rejection behavior.
No additional user-provided focus was supplied; the entire PR was reviewed.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29517 ms |
Contributor
TPC-DS: Total hot run time: 177568 ms |
Contributor
ClickBench: Total hot run time: 24.76 s |
gavinchou
approved these changes
Jul 31, 2026
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
5 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 original implementation has been replaced with the unified object-storage rate-limiting design accepted after review:
S3RateLimiterManagerowns the GET/PUT QPS and bandwidth buckets independently fromS3ClientFactory.s3_rate_limiter_cpu_corescan explicitly override the value; otherwise Doris combines hardware concurrency with the current cgroup CPU quota. A daemon refreshes the effective limits every 10 seconds so serverless CPU resizing and mutable config changes take effect.RateLimitedObjStorageClientapplies admission once per logical top-level operation. Paginated list requests and recursive deletes are therefore charged once instead of once per provider request. GET/PUT mapping is centralized and rejected requests return HTTP 429.-1; per-core limits support explicit disable and optional maximum caps.Implementation is based on and adapted from liaoxin01/doris@22f494a3.
Tests add deterministic coverage for config precedence, CPU-aware refresh, limiter state preservation, byte reservation/refund, logical pagination, recursive delete and Azure operation mapping, presigned URL bypass, factory wrapping policy, and full-key client-cache behavior.
Release note
Add CPU-aware GET/PUT QPS and bandwidth rate limiting for BE object-storage requests.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)