[improvement](be) Add CPU-aware S3 rate limiter#65420
Open
0AyanamiRei wants to merge 15 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
|
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 6:03 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: Existing coverage did not fully verify CPU-derived GET/PUT limits, independent QPS and bytes buckets, dynamic refresh, object-storage API mappings, or the real S3 SQL paths. Add focused BE unit and mapping tests, plus a non-cloud real-S3 regression that checks dynamic config readback, directional bvar growth, and concurrent GET/PUT behavior. ### Release note None ### Check List (For Author) - Test: Unit Test and Regression Test - `./run-be-ut.sh --run --filter=S3RateLimiterResolveTest.*:S3RateLimiterManagerTest.*:TokenBucketRateLimiterTest.rejected_add_rolls_back_count_and_tokens:S3RateLimitGuardTest.*:S3RateLimiterMetricsTest.*:RateLimitedObjStorageClientTest.* -j48` - `./run-regression-test.sh --run -d external_table_p2/s3_rate_limiter -s test_s3_rate_limiter -genOut` - `./run-regression-test.sh --run -d external_table_p2/s3_rate_limiter -s test_s3_rate_limiter` - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: Dynamic config validators were invoked before the converted value was published to the registered field, so validators that inspect the field validated the old value. This could allow an invalid S3 limiter value such as per_core=-2 through the HTTP update path. Assign the candidate before validation and restore the old value on failure. Expand S3 limiter unit coverage for invalid values, legacy fallback throttling, disabled GET/PUT accounting, oversized byte reservations, and extend the real-S3 regression suite with rejection metrics and direction-specific QPS rejection messages. ### Release note Invalid dynamic configuration updates are now rejected using the candidate value without changing the active configuration. ### Check List (For Author) - Test: Unit Test / Manual test - 40 focused BE unit tests passed. - ASAN BE build passed. - S3 regression was attempted but stopped during fixture creation because the private OSS credential returned InvalidAccessKeyId. - Behavior changed: Yes. Dynamic validators now validate the candidate value and restore the old value on failure. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: The previous regression used S3 TVF and Outfile in non-cloud mode, so it did not validate the Cloud internal-vault boundary where the limiter is enabled. Replace it with real internal OLAP scans and writes, isolate GET and PUT QPS/bytes phases, validate dynamic configuration and bvar deltas, and cover legacy token-limit rejection behavior. ### Release note None ### Check List (For Author) - Test: Regression test - `cloud_p0/s3/test_s3_rate_limiter` on a local three-BE Cloud cluster - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: S3FileReader retried BE-local rate limiter 429 responses through the normal object-storage retry path, but replaced the limiter-specific reason with a generic read failure after retries. Preserve the existing retry, sleep, and counter behavior while including the last limiter response in the final error. ### Release note S3 GET rate-limit rejection errors now retain the QPS or bytes limit reason after retries. ### Check List (For Author) - Test: Regression test - cloud_p0/s3/test_s3_rate_limiter - Behavior changed: Yes. Final GET limiter errors include the limiter-specific reason while retry and metrics behavior stay unchanged. - Does this need documentation: No
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: Add a Docker regression case that changes a running BE cgroup v2 CPU quota from one to two cores and verifies both cpu.max and the rebuilt GET/PUT QPS and bytes limiter parameters through BE logs. ### Release note None ### Check List (For Author) - Test: Regression test script loading; Docker execution was skipped because the current user cannot access the Docker socket - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
/review |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: The full BE UT suite cleared the global config registry in ConfigTest and did not restore it, causing later S3 limiter config tests to fail with missing fields. The performance build also rejected a designated ObjectStorageHeadResponse initializer under GCC with -Wmissing-field-initializers. Preserve and restore the config registry around ConfigTest, and construct the rate-limited head response explicitly. ### Release note None ### Check List (For Author) - Test: Unit Test - ConfigTest and the two previously failing S3 limiter tests in one process - 39 focused S3 limiter tests - BE clang-format, check-format, and clang-tidy - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
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: 29428 ms |
Contributor
TPC-DS: Total hot run time: 177076 ms |
Contributor
ClickBench: Total hot run time: 24.9 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65420 Problem Summary: StorageEngineTest.TestBrokenDisk declared a local broken_storage_path config with the same name as the production config. With the global config registry correctly preserved, persisting broken paths updates the registered production variable while the test asserted against the empty local variable. Remove the duplicate local definition so the test checks the registered configuration value. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=StorageEngineTest.TestBrokenDisk -j48 - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29469 ms |
Contributor
TPC-DS: Total hot run time: 177656 ms |
Contributor
ClickBench: Total hot run time: 24.97 s |
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
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)