Skip to content

enhance: use adaptive rate-limited retry strategy for S3 client - #590

Open
bigsheeper wants to merge 1 commit into
milvus-io:mainfrom
bigsheeper:fix/s3-adaptive-retry
Open

enhance: use adaptive rate-limited retry strategy for S3 client#590
bigsheeper wants to merge 1 commit into
milvus-io:mainfrom
bigsheeper:fix/s3-adaptive-retry

Conversation

@bigsheeper

Copy link
Copy Markdown

What & Why

The S3 client's default ConnectRetryStrategy used a fixed 200ms retry interval (up to ~30 attempts) and had no client-side rate limiting. Under sustained S3 request-rate throttling (503 SlowDown) — e.g. a large bulk import writing many small objects to a cold bucket prefix — this hammers the throttled endpoint at a constant rate and amplifies the overload.

This replaces it with a strategy built on the AWS SDK's AdaptiveRetryStrategy, which adds:

  • client-side rate limiting via a send token bucket that auto-enables on throttling responses (CUBIC/AIMD), pacing outgoing requests toward the endpoint's dynamic capacity;
  • jittered exponential backoff (inherited from the standard strategy), replacing the fixed 200ms interval.

The IsConnectError gate is preserved, so both AWS-standard retryable errors and MinIO-specific ones (SlowDown, XMinioServerNotInitialized) remain retriable.

Notes

  • The rate limiter is dormant under normal load and engages only once throttling is observed, so steady-state behavior is unchanged.
  • All S3 requests issued through this client (including CompleteMultipartUpload, which goes through the SDK's retry machinery) are paced by the limiter.
  • Verified against the aws-sdk-cpp 1.11.692 retry API. Runtime rate-limiting behavior under throttling is to be validated in integration.

🤖 Generated with Claude Code

Signed-off-by: bigsheeper <yihao.dai@zilliz.com>
@sre-ci-robot
sre-ci-robot requested review from sunby and tedxu July 24, 2026 08:58
@sre-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bigsheeper
To complete the pull request process, please assign tedxu after the PR has been reviewed.
You can assign the PR to them by writing /assign @tedxu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.48%. Comparing base (f9f5bd1) to head (5a997f9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #590      +/-   ##
==========================================
- Coverage   75.48%   75.48%   -0.01%     
==========================================
  Files         164      164              
  Lines       15818    15813       -5     
  Branches     2388     2388              
==========================================
- Hits        11941    11936       -5     
  Misses       3877     3877              
Flag Coverage Δ
python 44.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Thanks for working on this. I suggest treating retry behavior as an object-storage-wide policy rather than an S3-only implementation detail:

  1. Please cover all supported object-storage backends, including non-S3-native paths such as Azure Blob/ADLS. The scope should also explicitly state whether credential/STS requests, custom retry strategies, CRT reads, and multipart-upload retry paths are included.
  2. Please audit each cloud SDK and align the retry semantics across providers: retryable error classification (429/503, timeout, connection reset, service-not-ready), exponential backoff and jitter, Retry-After, maximum attempts and total elapsed-time budget, client-side rate-limiter scope, and idempotency requirements.

“Consistent” should mean a common behavioral contract, not necessarily identical SDK classes or constants. Where a provider SDK cannot provide equivalent adaptive throttling, we should add a provider adapter or a common limiter above the SDK.

Please also add provider-specific fault-injection tests for AWS S3, MinIO, GCS, Aliyun OSS, Tencent COS, Huawei OBS, and Azure to verify the full path from the real SDK error mapping to retry, backoff, and rate limiting.

If this is too large for this PR, please clearly scope this PR as S3-compatible-only and track the Azure/native-SDK and credential-path work as explicit follow-ups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants