Skip to content

feat(layer): add throttle layer#2444

Merged
Xuanwo merged 4 commits intoapache:mainfrom
morristai:feat/add_throttle_support
Jun 13, 2023
Merged

feat(layer): add throttle layer#2444
Xuanwo merged 4 commits intoapache:mainfrom
morristai:feat/add_throttle_support

Conversation

@morristai
Copy link
Copy Markdown
Member

@morristai morristai commented Jun 10, 2023

Description

Background

NOTE: This is still a early stage of this PR, I open this PR for RFC and guidance.

As there's no open-source repository that uses Governor to limit bandwidth so far IMK, it's mostly used to limit the number of requests, e.g. tower-governor (although Governor can theoretically limit any element).

There's an issue asked by Quickwit's author about how to use Governor to limit IO throughput, but he ended up using async-speed-limit for Quickwit (PR, which also seems to use a truncated byte stream implementation.).

My current implementation uses the answer provided in the above issue by the Governor author. However, it's rough and untested, and there are some uncertainties that I left comments inline. Things I need more information to confirm include:

  • Is it possible to limit read (async/blocking) operations since the layer can't affect the inner.read() implementor's behavior?(I assume)?

Answer by Xuanwo: "We can get the read size and wait for them. But it may be complex for waiting in poll_read. We can finish it in a new PR."

  • For my current implementations for Write (async/blocking) and Append operations, I use the Bytes truncation method to limit inner consumption. I don't know if this direction is the right approach so far.

Answer by Xuanwo: #2444 (comment)

@morristai morristai marked this pull request as draft June 10, 2023 04:59
@github-actions github-actions Bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Jun 10, 2023
@morristai morristai force-pushed the feat/add_throttle_support branch from 6dbe3e9 to a917016 Compare June 10, 2023 05:02
Comment thread core/src/layers/throttle.rs Outdated
Comment thread core/src/layers/throttle.rs Outdated
Comment thread core/src/layers/throttle.rs Outdated
Comment thread core/src/layers/throttle.rs Outdated
@morristai morristai force-pushed the feat/add_throttle_support branch from a917016 to 9a7bf48 Compare June 13, 2023 01:36
@morristai morristai requested a review from Xuanwo June 13, 2023 01:37
Copy link
Copy Markdown
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, only some naming related issues.

Comment thread core/src/layers/throttle.rs Outdated
Comment thread core/src/layers/throttle.rs Outdated
Comment thread core/src/layers/throttle.rs Outdated
@morristai morristai marked this pull request as ready for review June 13, 2023 01:52
Copy link
Copy Markdown
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@morristai
Copy link
Copy Markdown
Member Author

BTW, there are still 2 todo item comments inside this PR:

  • Can we apply a throttle layer over oio::Read/oio::BlockingRead operations?
  • When the query is valid but the Decider cannot accommodate it, should we lock the limiter and wait for the wait_time, or should we let other small requests go first?

If you believe that they are not essential, then I will remove them.

@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Jun 13, 2023

  • Can we apply a throttle layer over oio::Read/oio::BlockingRead operations?

We can get the read size and wait for them. But it may be complex for waiting in poll_read. We can finish it in a new PR.

  • When the query is valid but the Decider cannot accommodate it, should we lock the limiter and wait for the wait_time, or should we let other small requests go first?

Current behavior LGTM.

@Xuanwo Xuanwo merged commit 023fd08 into apache:main Jun 13, 2023
@PsiACE PsiACE mentioned this pull request Jun 27, 2023
3 tasks
@morristai morristai deleted the feat/add_throttle_support branch May 27, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Throttle Support

3 participants