[fix](s3) Add S3 rate limit observability#64038
Merged
liaoxin01 merged 5 commits intoJul 15, 2026
Merged
Conversation
gavinchou
requested review from
Gabriel39,
liaoxin01,
luwei16 and
morningman
as code owners
June 2, 2026 18:16
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found coverage gaps in the new S3 throttling observability. The BE object-client path uses the new helper/counters, but the Cloud recycler has parallel S3/Azure object-client paths that still use the old limiter metrics and do not record final 429 failures. This means the new bvars can under-report or completely miss recycler throttling, even though the recycler uses the same retry strategy and is an important object-storage consumer.
Critical checkpoint conclusions:
- Goal/test: The PR adds S3 local limiter and 429 observability, with unit coverage for the holder metrics, but the goal is only partially met because Cloud recycler paths are not covered.
- Scope/focus: The change is generally focused, but parallel object-storage paths were missed.
- Concurrency/lifecycle: New atomic log counters and bvar adders are safe for concurrent updates; no new lock-order risk found.
- Config: The new mutable log interval has validation and is read dynamically where the BE helper is used.
- Compatibility: Bvar renames are user-visible; Cloud still exposes old names, so behavior is inconsistent across binaries.
- Parallel code paths: Cloud recycler S3/Azure clients are parallel paths and need the same accounting.
- Tests: Existing tests cover only the generic holder metric callback, not BE vs Cloud end-to-end metric coverage.
- Observability: Needs fixes below so the new observability is complete.
User focus: No additional user-provided review focus was specified.
| return [&](int64_t ns) { | ||
| inline auto metric_func_factory(bvar::Adder<int64_t>& sleep_ns_bvar, | ||
| bvar::Adder<int64_t>& sleep_count_bvar, | ||
| bvar::Adder<int64_t>* rejected_count_bvar = nullptr) { |
Contributor
There was a problem hiding this comment.
Making the rejected counter optional leaves an existing S3 limiter path without the new metric. cloud/src/recycler/s3_accessor.cpp still constructs its AccessorRateLimiter with the old two-argument call (get_rate_limit_ns, get_rate_limit_exceed_req_num, etc.), and cloud/src/recycler/s3_obj_client.cpp / azure_obj_client.cpp call AccessorRateLimiter::instance().rate_limiter(op)->add(1) directly. As a result, recycler local limiter rejections still do not increment s3_get_rate_limit_rejected_count / s3_put_rate_limit_rejected_count, and the newly added throttled/rejected logging in apply_s3_rate_limit() is bypassed there. This contradicts the PR's stated metric rename/new rejection metric behavior for S3 local limiting. Please update the Cloud recycler accessor path to use the renamed bvars and pass rejection counters, or route it through a shared helper with the same logging/accounting.
| bvar::Adder<int64_t> s3_request_failed_too_many_requests_count( | ||
| "s3_request_failed_too_many_requests_count"); | ||
|
|
||
| void record_object_request_failed(int http_code) { |
Contributor
There was a problem hiding this comment.
The new final-429 failure counter is common, but only the BE object clients call record_object_request_failed(). The Cloud recycler also installs S3CustomRetryStrategy / AzureRetryRecordPolicy in cloud/src/recycler/s3_accessor.cpp, so retry 429s from recycler requests are counted, but final 429 failures in cloud/src/recycler/s3_obj_client.cpp and azure_obj_client.cpp just log/return without calling this function. That makes s3_request_failed_too_many_requests_count under-report exactly the recycler failures that still exhausted retries. Please add the same failure recording to the Cloud S3/Azure non-success paths (excluding expected not-found handling) so the retry and failed 429 metrics cover the same object-client paths.
Contributor
TPC-H: Total hot run time: 29261 ms |
Contributor
TPC-DS: Total hot run time: 168900 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Collaborator
|
Consider supporting recyclers as well? A general framework could be written in doris/common |
gavinchou
force-pushed
the
gavin-log-s3-rate-limit-observability
branch
from
June 4, 2026 14:54
e989d8a to
ac67923
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Author
|
Addressed the review comments in ac67923.
Validation:
|
Contributor
There was a problem hiding this comment.
I found one blocking concurrency issue in the new S3 rate-limiter logging path. The existing recycler metric coverage comments are already-known context and appear addressed in this revision, so I did not duplicate them.
Critical checkpoint conclusions:
- Goal/test: the PR adds S3/Azure retry/final-429 metrics and local limiter sleep/rejection metrics/logging, with unit coverage for metric accounting. The goal is mostly met, but the logging implementation introduces an unsafe concurrent access.
- Scope/focus: the change is focused on object-storage retry/rate-limit accounting.
- Concurrency: affected.
reset_s3_rate_limiter()can replaceTokenBucketRateLimiterHolder::rate_limiterunder the write lock while request threads callapply_s3_rate_limit(). The new log code reads holder state afteradd()releases the read lock, creating a race/use-after-free risk. - Lifecycle/static initialization: no additional cross-TU static initialization issue found beyond existing global bvars.
- Configuration:
s3_rate_limiter_log_intervalis added with validation and is read dynamically at call sites. - Compatibility: no storage/protocol compatibility issue found.
- Parallel paths: BE S3/Azure and Cloud recycler S3/Azure paths were reviewed; prior missing Cloud paths are covered in this revision.
- Conditional checks/error handling: expected not-found handling is preserved for the reviewed object-client paths.
- Test coverage: metric unit coverage exists, but no concurrent reset/logging coverage protects the race below.
- Observability/performance: new metrics/logging are useful, but the log state reads must be made concurrency-safe.
- Transactions/persistence/data writes/FE-BE variables: not applicable to this PR.
User focus response: no additional user-provided focus was specified.
| << ", sleep_ms=" << sleep_duration / 1000000 << ", sleep_count=" << count | ||
| << ", token_per_second=" << rate_limiter->get_max_speed() | ||
| << ", bucket_tokens=" << rate_limiter->get_max_burst() | ||
| << ", token_limit=" << rate_limiter->get_limit(); |
Contributor
There was a problem hiding this comment.
This log path now reads rate_limiter->get_max_speed() / get_max_burst() / get_limit() after TokenBucketRateLimiterHolder::add() has returned and released its shared_lock. Both BE and Cloud expose reset_s3_rate_limiter() paths that take the holder write lock and replace the underlying unique_ptr (rate_limiter = std::make_unique<...>). A request thread that reaches this log branch can therefore race with a dynamic reset and dereference the replaced/freed limiter through these unlocked getters. Please keep the logged configuration snapshot under the same holder lock as add() (or return/snapshot the values from add() while locked) before logging.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29817 ms |
Contributor
TPC-DS: Total hot run time: 169463 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Collaborator
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29718 ms |
Contributor
TPC-DS: Total hot run time: 169618 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
github-actions
Bot
removed
the
approved
Contributor
There was a problem hiding this comment.
Requesting changes for one remaining observability gap: BE Azure batch delete subresponse failures can still bypass the new final-429 failure counter.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds S3 local limiter and 429 observability, but the accepted inline issue leaves one BE Azure final-failure path undercounted. The added unit coverage checks limiter sleep/rejected metrics; I did not find coverage for this BE Azure batch failure path.
- Scope/focus: the implementation is otherwise focused on S3/Azure rate-limiter logging, bvars, retry/failure accounting, config, and a Cloud rate-limiter unit test.
- Concurrency/lifecycle: the known unlocked limiter logging/reset race remains duplicate of existing thread discussion_r3356989298, so I did not duplicate it.
- Config/compatibility: the new BE and Cloud log interval configs are mutable and read at request time. Metric renames are user-visible observability changes but do not affect storage or FE/BE protocol compatibility.
- Parallel paths: BE and Cloud S3/Azure paths were compared. Cloud Azure batch deferred failures record the helper; the BE Azure batch path is the remaining gap called out inline.
- Transaction/persistence/data correctness: not applicable to this PR; no Doris transaction, storage-format, or persisted metadata path is changed.
- Performance/observability: logging is interval-gated; the inline issue affects observability accuracy for 429 failures.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite found no optimizer/planner relevance and no new findings. tests-session-config marked the unlocked logging race as duplicate of existing thread discussion_r3356989298 and independently confirmed the BE Azure batch issue as duplicate of MAIN-001. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29165 ms |
Contributor
TPC-DS: Total hot run time: 179958 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one remaining correctness issue in the observability path: Azure 429 retries can still be under-counted when the first 429 is retried successfully.
Critical checkpoint conclusions:
- Goal/test proof: the PR mostly achieves the S3 local limiter and final 429 observability goal, and the current head fixes the earlier Cloud/BE accounting and logging-race comments. The remaining gap is the Azure retry-success path; the added tests do not cover that pipeline case.
- Scope/focus: the change is focused on object-storage rate limiting, retry metrics, configs, and tests.
- Concurrency/lifecycle: the previous unlocked limiter logging snapshot race is addressed by
add_with_config()taking the holder lock while reading limiter config. - Config: the new mutable log interval is read at request time; existing token/bucket/limit reset paths are preserved.
- Compatibility: no storage or wire-format compatibility issue found. Metric renames are user-visible and documented in the PR body.
- Parallel paths: BE S3, BE Azure, Cloud S3, Cloud Azure, and batch-delete final-failure paths were checked. Existing review threads cover and appear resolved for the prior propagation/final-failure/race issues; MAIN-001 is distinct.
- Error handling/status: final non-not-found S3/Azure failures now call the new failure recorder in the reviewed paths. No ignored
Statusissue was found in the changed code. - Testing/validation: I did not run BE/Cloud tests locally because this grafted CI checkout is missing
.worktree_initializedandthirdparty/installed. This is a static review against the live GitHub patch and checked-out head. - Observability: the new bvars and logs are mostly wired through, but the Azure retry counter still misses a successful-retry throttling pattern.
- User focus: no additional user-provided focus was supplied.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite, semantic-equivalence, or parallel join/aggregate issue found.tests-session-config: no additional non-duplicate test/config/style issue found.- Convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the final ledger/comment set containing only MAIN-001.
| @@ -65,6 +78,10 @@ std::unique_ptr<Azure::Core::Http::RawResponse> AzureRetryRecordPolicy::Send( | |||
| static_cast<int>(response->GetStatusCode()) < 200) { | |||
| if (retry_count > 0) { | |||
| object_request_retry_count << 1; | |||
Contributor
There was a problem hiding this comment.
This still misses the common Azure throttling case where the first attempt returns 429 and the SDK retry succeeds. AzureRetryRecordPolicy is installed in PerRetryPolicies, which run downstream of RetryPolicy for each attempt; on the first 429 response, GetRetryCount(context) is still 0, so this if (retry_count > 0) block does not increment s3_request_retry_too_many_requests_count. If the next attempt succeeds, no later non-2xx response passes through this block, so the new 429 retry metric stays flat while S3 increments it from ShouldRetry on the first retryable 429. Please record the 429 before this retry-count gate, or otherwise hook the Azure retry decision path, so successful retries after an initial 429 are counted consistently.
Contributor
TPC-H: Total hot run time: 29439 ms |
Contributor
TPC-DS: Total hot run time: 179820 ms |
Contributor
ClickBench: Total hot run time: 25.32 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
0AyanamiRei
added a commit
to 0AyanamiRei/doris
that referenced
this pull request
Jul 15, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#64038 Related PR: apache#65420 Problem Summary: PR apache#64038 refactored S3 rate limiting around a common observability helper and added throttling and 429 metrics, while this branch added CPU-aware QPS and byte limits, cloud traffic provenance, and non-blocking dynamic reset. Merge master and combine both designs so each request is charged once, the new observability remains active, and CPU-aware and internal traffic semantics are preserved. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=S3UTILTest.*:S3ClientFactoryTest.*:S3FileWriterTest.* -j48 - ./run-cloud-ut.sh --run --filter=s3_rate_limiter_test:* -j48 - ./build-support/check-format.sh - Behavior changed: No. This resolves integration with the latest S3 limiter observability refactor while preserving the PR behavior. - Does this need documentation: No
yiguolei
pushed a commit
that referenced
this pull request
Jul 24, 2026
### What problem does this PR solve? Backport #64038 to `branch-4.1`. Add observability for S3 local rate limiting and S3/Azure 429 responses so throttling-related performance issues can be diagnosed and alerted more directly. ### What is changed? - Add explicit S3 local rate limiter bvars for sleep duration, sleep count, and rejected count. - Rename the old ambiguous S3 local rate limiter bvars so their meaning is clear. - Log the first and every N local S3 rate limiter throttled/rejected requests without reading bvar values for decisions. - Add S3 429 retry/failure bvars for S3/Azure object clients, including Azure batch delete failures. - Capture the limiter configuration and result under the limiter lock to avoid concurrent reset races. - Adapt the Cloud configuration change to `branch-4.1` without bringing in unrelated master-only fault-injection settings. ### New or renamed bvar metrics Renamed local S3 GET limiter metrics: - `get_rate_limit_ns` -> `s3_get_rate_limit_sleep_ns` - `get_rate_limit_exceed_req_num` -> `s3_get_rate_limit_sleep_count` Renamed local S3 PUT limiter metrics: - `put_rate_limit_ns` -> `s3_put_rate_limit_sleep_ns` - `put_rate_limit_exceed_req_num` -> `s3_put_rate_limit_sleep_count` New local S3 limiter rejection metrics: - `s3_get_rate_limit_rejected_count` - `s3_put_rate_limit_rejected_count` New S3/Azure 429 metrics: - `s3_request_retry_too_many_requests_count` - `s3_request_failed_too_many_requests_count` Metric examples: ```text s3_get_rate_limit_sleep_ns : 123456789 s3_get_rate_limit_sleep_count : 42 s3_get_rate_limit_rejected_count : 3 s3_put_rate_limit_sleep_ns : 987654321 s3_put_rate_limit_sleep_count : 27 s3_put_rate_limit_rejected_count : 1 s3_request_retry_too_many_requests_count : 8 s3_request_failed_too_many_requests_count : 2 ``` Example local limiter logs: ```text S3 GET request is throttled by local rate limiter, sleep_ms=12, sleep_count=1000, token_per_second=1000, bucket_tokens=2000, token_limit=5000 S3 PUT request is rejected by local rate limiter, rejected_count=1, token_per_second=1000, bucket_tokens=2000, token_limit=5000 ``` ### Tests - `sh run-cloud-ut.sh --run --filter='s3_rate_limiter_test:*'` (7/7 passed) - `sh run-be-ut.sh --run --filter='S3UTILTest.*:S3FileWriterTest.*'` (24/24 passed) - `sh run-be-ut.sh --run --filter='AzureObjStorageClientTlsHelperTest.*'` (2/2 passed) Co-authored-by: gavinchou <gavinchou@apache.org>
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?
Add observability for S3 local rate limiting and S3 429 responses so throttling-related performance issues can be diagnosed and alerted more directly.
What is changed?
New or renamed bvar metrics
Renamed local S3 GET limiter metrics:
get_rate_limit_ns->s3_get_rate_limit_sleep_nsget_rate_limit_exceed_req_num->s3_get_rate_limit_sleep_countRenamed local S3 PUT limiter metrics:
put_rate_limit_ns->s3_put_rate_limit_sleep_nsput_rate_limit_exceed_req_num->s3_put_rate_limit_sleep_countNew local S3 limiter rejection metrics:
s3_get_rate_limit_rejected_counts3_put_rate_limit_rejected_countNew S3 429 metrics:
s3_request_retry_too_many_requests_counts3_request_failed_too_many_requests_countMetric examples:
Example local limiter logs:
Tests
sh run-cloud-ut.sh --run --filter=s3_rate_limiter_test:*sh run-be-ut.sh --run --filter=S3FileWriterTest.*Note: an earlier BE run used an incorrect lowercase filter (
s3_file_writer_test:*), which matched no suite and started running the full BE suite; it was stopped and replaced by the correctS3FileWriterTest.*run above.