[improvement](be) Add CPU-aware S3 rate limiter#65420
Open
0AyanamiRei wants to merge 3 commits into
Open
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 |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Jul 17, 2026
### 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 |
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Jul 17, 2026
### 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 |
…decorator
Redesign the S3 GET/PUT rate limiting path:
- New configs s3_{get,put}_qps_per_core / _qps_max and
s3_{get,put}_bytes_per_second_per_core / _max derive per-BE limits from
CPU cores (cgroup-quota aware), so heterogeneous BEs get proportional
budgets. -1 keeps the legacy absolute token configs bit-for-bit.
- All rate limiting now lives in one decorator, RateLimitedObjStorageClient,
wrapped by S3ClientFactory at construction time. Provider clients (S3,
Azure, future GCP) contain no rate limiting code anymore.
- In cloud mode only internal storage-vault buckets are limited; external
buckets (S3 load, TVF, external catalogs) get the bare client. Non-cloud
mode wraps every client, preserving legacy behavior.
- New bytes-per-second buckets charge payload size with reserve+settle
accounting: short reads are refunded, single reservations are clamped to
1s of bandwidth, and the guard pins the exact bucket generation it charged
so a concurrent limiter reset cannot make the refund pollute a fresh
bucket. The derived per-BE bytes/s should not be set below the single IO
upper bound per second (s3_write_buffer_size); documented on the configs.
- S3RateLimiterManager::refresh() is idempotent and driven by a daemon
thread, picking up both dynamic config changes (now effective in non-cloud
mode too) and in-place cgroup CPU resizes for serverless BEs; a positive
s3_rate_limiter_cpu_cores overrides detection for control-plane pushes.
- Fix S3 client cache and ObjClientHolder::reset treating get_hash()
equality as configuration identity (XOR of crc32s collides easily).
- Fix TokenBucketRateLimiterHolder sleeping while holding the read lock,
which blocked reset() for the whole throttle duration.
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: After adopting the unified object-storage rate limiter implementation, deterministic coverage was needed for refresh state, CPU-based resizing, reservation settlement, logical pagination and operation mapping, factory wrapping policy, and full-key client cache behavior. This change adds that coverage, removes wall-clock-sensitive assertions, and makes dynamic config restoration safe on early test exits. ### Release note None ### Check List (For Author) - Test: Unit Test / Build - Targeted BE unit tests: 28 passed, 1 skipped because the S3 client is disabled locally - BUILD_TYPE=Debug ./build.sh --fe --be -j64 - build-support/check-format.sh - Debug BE UT objects compiled; final link is blocked by the pre-existing unresolved __lsan_disable/__lsan_enable references in cached_remote_file_reader_peer_test.cpp - Behavior changed: No. Test coverage only. - Does this need documentation: No
0AyanamiRei
force-pushed
the
feature/cpu-aware-s3-limiter
branch
from
July 20, 2026 01:50
d538f61 to
a12fe2e
Compare
Contributor
Author
|
run buildall |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: five correctness issues remain in the new limiter path: provider-request QPS is no longer bounded for pagination/batching, a permanent local rejection is mislabeled as retryable 429, oversized I/O bytes are dropped from accounting, token refill/refund ordering permits excess bursts, and refresh can split one reservation across two limiter generations.
Critical checkpoint conclusions:
- Goal and proof: The PR centralizes CPU-aware GET/PUT QPS and bandwidth limiting for S3/Azure, adds dynamic refresh, and distinguishes cloud-internal clients. The manager/decorator/factory structure implements that shape, but the five issues above mean it does not yet enforce the advertised limits correctly. The unit tests cover config resolution, refresh, wrapping, cache identity, provider mapping, and refund mechanics, but some tests codify the defective logical-call/clamp semantics and do not prove production traffic bounds.
- Scope and clarity: The change is focused on object-storage rate limiting and mostly reuses one decorator/manager abstraction. No unrelated source change was found.
- Concurrency: I traced I/O workers racing the daemon refresh through holder shared mutexes, per-bucket spinlocks, and pinned shared pointers. No deadlock, UAF, or unnecessarily heavy operation under a lock was found, but the separate
get_max_speed()andcharge()snapshots create the reported cross-generation race. The previously reported retired-generation debt issue remains real and was not duplicated here. - Lifecycle: Daemon startup/stop and singleton/holder lifetimes terminate cleanly; no circular ownership, shutdown hazard, or cross-TU static-initialization dependency was found.
- Configuration and compatibility: Mutable limits are refreshed and the legacy absolute settings remain a fallback, but valid low bandwidth settings contradict the unenforced single-I/O assumption, and moving fallback QPS admission from provider calls to logical calls changes existing behavior. No wire/storage format or rolling-upgrade compatibility boundary is changed.
- Parallel paths and conditions: S3 and Azure construction, cloud vault create/reset, external-client exclusion, cache equality/hash, and non-cloud wrapping were traced. Internal/external classification is complete, but S3/Azure list pages, recursive-delete GET/PUT calls, Azure batches, and Azure's no-op multipart-create path are not accounted at actual request boundaries.
- Tests and expected results: No regression-result files change. Missing negative/concurrency coverage is called out inline: real provider page/batch counts, reader-level hard rejection, I/O larger than one second of quota, controllable-clock idle/refund cases, and barrier tests across rate refresh.
- Observability and error handling: Throttle logs and bvar metrics exist and are lightweight enough. The local hard rejection's 429 mapping is incorrect and also pollutes provider-throttle retry/metrics behavior; no other unchecked status or exception-boundary issue was found.
- Transactions, persistence, data writes, and FE/BE propagation: No EditLog, transaction protocol, persistent metadata, FE/BE field propagation, nullable-column, or allocator-sensitive ownership change is involved. Object writes remain delegated to the provider clients; the defects concern admission/timing rather than storage atomicity.
- Performance: Disabled-holder fast paths are present, but the 429 mapping can cause futile exponential backoff, the refresh race can cause very long oversleeps, and logical-call QPS can generate provider bursts. No additional CPU/memory complexity issue survived review.
- Other/user focus: Existing comments about SDK retry-byte accounting, reset debt, CPU-source selection, and signing order were deduplicated. No additional user focus was supplied.
Validation was static only, as required by this review runner; I did not build or run tests. At submission time, formatter, license, large-file, FE UT, and Cloud UT checks pass; BE UT, compile, performance, and this code-review check are still pending.
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 S3 byte limiter could admit an unbounded amount of reserved work. Each admitted request added token debt before sleeping, so bursts could produce progressively longer waits while occupying object-storage worker threads. This change reuses the limiter count as the outstanding byte reservation, caps it at one second of effective bandwidth, atomically rolls back rejected charges, and separates short-read token settlement from request-completion count release. ### Release note Bound outstanding S3 byte-rate reservations to prevent unbounded limiter waits. ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=TokenBucketRateLimiterTest.*:S3RateLimiterManagerTest.*:S3RateLimitGuardTest.*:RateLimitedObjStorageClientTest.* -j48 - Behavior changed: Yes. S3 byte reservations exceeding one second of effective bandwidth are rejected without leaving byte-token or count debt. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 25th, 2026 3:35 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
4 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
BUILD_TYPE=Debug ./build.sh --fe --be -j64build-support/check-format.shBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)