Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RateLimitPolicy #903

Merged

Conversation

martincostello
Copy link
Member

The issue or feature being addressed

#260 Rate limiting

This PR takes all the changes from #666 which was originally authored by @reisenberger and makes the following additional changes:

  1. Removes the internal unused LockBasedTokenBucketRateLimiter class.
  2. Throws ArgumentNullException instead of NullReferenceException.
  3. Passes the relevant value to the constructor when ArgumentOutOfRangeException is thrown.
  4. Makes two internal classes static or sealed.
  5. Makes a minor readability change.
  6. Changes the code to consistently use string over String.
  7. Fixes a typo in an exception message.
  8. Bumps the version to 7.3.0.

We've been using the original rate-limiter changes from #666 successfully in production at Just Eat since March 2020 and have had no issues with it, and it has served our use case well. Via that usage it has also gone through a lot of production usage at scale, so based on our experiences I would consider the changes in this PR ready as-is.

As @reisenberger isn't currently available to commit his time to this project to complete the work, I'm going to treat this PR as the successor, but it is still open for additional review by the community before it is merged. Comments are welcome and encouraged.

I propose merging this in January 2022 for a Polly v7.3.0 release if there are no objections and no further feedback that should be addressed. I feel a month for further comments and review is a reasonable period for this, considering the original PR has had no new feedback (other than "when will this be merged?" 😃) since March 2021.

Confirm the following

  • I started this PR by branching from the head of the latest dev vX.Y branch, or I have rebased on the latest dev vX.Y branch, or I have merged the latest changes from the dev vX.Y branch
  • I have targeted the PR to merge into the latest dev vX.Y branch as the base branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

reisenberger and others added 30 commits March 23, 2019 20:18
Remove the LockBasedTokenBucketRateLimiter class which isn't used other than in tests.
Throw ArgumentNullException instead of NullReferenceException.
Pass the invalid value when throwing ArgumentOutOfRangeException.
Seal the LockFreeTokenBucketRateLimiter class.
Make the RateLimiterFactory class static.
Add parenthesis to operation to make it clearer.
Use the always-unambiguous `string` keyword instead of `String`.
See https://blog.paranoidcoding.com/2019/04/08/string-vs-String-is-not-about-style.html.
Bump the next version to 7.3.0.
Remove superfluous dollar character from exception message.
@martincostello martincostello added this to the v7.3.0 milestone Dec 2, 2021
This was referenced Dec 2, 2021
README.md Outdated Show resolved Hide resolved
src/Polly/RateLimit/IRateLimitPolicy.cs Outdated Show resolved Hide resolved
src/Polly/RateLimit/LockFreeTokenBucketRateLimiter.cs Outdated Show resolved Hide resolved
src/Polly/RateLimit/LockFreeTokenBucketRateLimiter.cs Outdated Show resolved Hide resolved
Fix exception message typos.
Add access modifier.
Remove blank line after comment.
@martincostello
Copy link
Member Author

If there's no feedback forthcoming, I'll aim to move forward with this on or after Monday 17th January.

README.md Outdated Show resolved Hide resolved
Change ratelimit to rate-limit.
Fix various typos, as well as free whitespace trimming from using Atom.
Add documentation for the RateLimit policy.
Update to the highest 0.x version of Cake and pin to use a version of Cake.FileHelpers and Cake.Yaml released in the same time frame.
Suppress a NuGet package being created by the benchmarks project.
@martincostello martincostello merged commit 720f4ab into App-vNext:v723-or-v730 Jan 17, 2022
@martincostello martincostello deleted the Rate-Limiter-Policy branch January 17, 2022 08:41
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.

Feature request: rate-limiting (requests/time)
2 participants