Skip to content

fix(layers/throttle): precheck bounded read quota#7453

Open
TennyZhuang wants to merge 2 commits into
apache:mainfrom
TennyZhuang:codex/throttle-read
Open

fix(layers/throttle): precheck bounded read quota#7453
TennyZhuang wants to merge 2 commits into
apache:mainfrom
TennyZhuang:codex/throttle-read

Conversation

@TennyZhuang
Copy link
Copy Markdown
Contributor

@TennyZhuang TennyZhuang commented Apr 27, 2026

Which issue does this PR close?

Closes #7328.

Rationale for this change

The throttle layer applies byte quota accounting to writes, but read operations were not covered. A maintainer review clarified that post-read accounting is not useful for production workloads because the request has already been sent and bursty range reads are still possible.

This PR therefore narrows the read-side behavior to bounded/range reads where OpRead::range().size() is known before sending the request. Those reads can reserve quota before delegating to the backend, matching the existing write-side "check before send" semantics.

Unbounded/full-object reads remain out of scope because there is no requested byte count available before calling the inner accessor. The PR intentionally does not pretend to throttle those reads after the response has already arrived.

What changes are included in this PR?

  • Pre-check quota for bounded reads before calling the inner accessor.
  • Leave unbounded reads forwarded without read-side throttling.
  • Keep write-side quota behavior unchanged.
  • Add focused tests proving bounded reads within burst are forwarded, bounded reads over burst fail before the inner read request is sent, and unbounded reads are forwarded.

Validation:

git diff --name-status origin/main...HEAD
M core/Cargo.lock
M core/layers/throttle/Cargo.toml
M core/layers/throttle/src/lib.rs

cargo fmt --check
cargo test -p opendal-layer-throttle read_
cargo clippy -p opendal-layer-throttle --all-targets -- -D warnings

Are there any user-facing changes?

No API change. Bounded/range reads now participate in throttle quota checks before requests are sent.

AI Usage Statement

This PR was prepared with AI assistance from OpenAI Codex (GPT-5), supervised by @TennyZhuang in the staging regression workflow.

Copy link
Copy Markdown
Contributor Author

@TennyZhuang TennyZhuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-review from @clara-claude-pyreview-719124 (Python bindings / staging regression).

Change: extracts wait_for_quota helper shared by read and write, then applies it to reads (previously only writes were throttled).

Read throttle semantics: quota is checked after the read completes (inner.read().await? then wait_for_quota). This is correct — you can't know the byte count before reading. The tradeoff is that data is already in memory if the quota is exceeded, but that's unavoidable for streaming reads.

Refactor: wait_for_quota cleanly deduplicates the len == 0, len > u32::MAX, and until_n_ready logic. Write path is unchanged in behavior.

Tests: read_allows_chunks_within_burst and read_rejects_chunks_larger_than_burst cover both paths. tokio dev-dep addition is correct.

No concerns. LGTM.

@TennyZhuang TennyZhuang marked this pull request as ready for review April 27, 2026 19:03
@TennyZhuang TennyZhuang requested a review from Xuanwo as a code owner April 27, 2026 19:03
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Apr 27, 2026
Comment thread core/layers/throttle/src/lib.rs Outdated
@TennyZhuang
Copy link
Copy Markdown
Contributor Author

Staging cross-review note from @clara-claude-pyreview-719124:

LGTM. The shared quota helper is clean, and applying quota after each read buffer is the correct throttle-layer behavior for this internal oio::Read API. The focused read-side tests cover chunks within burst and chunks larger than burst.

Local validation reported by author:

  • cargo fmt --check
  • cargo test -p opendal-layer-throttle read_
  • cargo clippy -p opendal-layer-throttle --all-targets -- -D warnings

CI classification: remaining failures are unrelated infra. OCaml jobs fail with the known opam release constraint error. The S3 anonymous behavior job fails before tests because curl -O https://dl.min.io/client/mc/release/linux-amd64/mc downloads a 141-byte response and then ./mc: line 1: a: No such file or directory.

@TennyZhuang TennyZhuang force-pushed the codex/throttle-read branch 2 times, most recently from e59eb51 to 7ea86d6 Compare April 28, 2026 03:31
@TennyZhuang TennyZhuang changed the title fix(layers/throttle): limit read bandwidth fix(layers/throttle): precheck bounded read quota Apr 28, 2026
@TennyZhuang TennyZhuang force-pushed the codex/throttle-read branch from 02fb453 to 103cdca Compare April 28, 2026 04:58
@asukaminato0721 asukaminato0721 enabled auto-merge (squash) April 30, 2026 14:57
.read(path, args)
.await
.map(|(rp, r)| (rp, ThrottleWrapper::new(r, limiter)))
self.inner.read(path, args).await
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious do you think it worth a stats call before IO?
I think the whole purpose of this layer is to prevent excessive client / server resource usage, but current implementation could slip through large IO request, which beats the purpose.

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

Labels

releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new feature: add throttle for more operations

2 participants