Skip to content

Rate limiting /1 #26756

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

Merged
merged 43 commits into from
Aug 30, 2022
Merged

Rate limiting /1 #26756

merged 43 commits into from
Aug 30, 2022

Conversation

Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Aug 17, 2022

Contributes to #26515
Internal review URL

Public review of the doc build:

  • download the HTML files
  • unzip
  • view HTML file in a browser.
  • For links outside the doc, remove the review. from URL.

@Rick-Anderson
Copy link
Contributor Author

Rick-Anderson commented Aug 24, 2022

@pentp @martincostello Public review of the doc build:
download the HTML files
unzip
view HTML file in a browser.

@Rick-Anderson
Copy link
Contributor Author

Rick-Anderson commented Aug 25, 2022

@IEvangelist I did a quick write up on the algorithms with the idea when you publish the .NET rate limiting doc, I'll point to your doc. I really don't want to invest more time explaining them since I'm going to remove the explanation from this doc. I'm sure you'll have a better write up, feel free to make suggestions here.

@wtgodbe
Copy link
Member

wtgodbe commented Aug 26, 2022

@IEvangelist I did a quick write up on the algorithms with the idea when you publish the .NET rate limiting doc, I'll point to your doc. I really don't want to invest more time explaining them since I'm going to remove the explanation from this doc. I'm sure you'll have a better write up, feel free to make suggestions here.

@BrennanConroy also did a great writeup of the algorithms here: https://devblogs.microsoft.com/dotnet/announcing-rate-limiting-for-dotnet/

@Rick-Anderson Rick-Anderson requested a review from wtgodbe August 26, 2022 21:50
| 30 | **[+20]** | | | -30 | | | |
| 40 | |**[+30]**| | | -10 | | |
| 50 | | | **[+40]** | | | -10 | |
| 60 | | | | **[+30]** | | | -35|
Copy link
Member

Choose a reason for hiding this comment

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

I think this table is too confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about something like my updated table? If you don't like that approach, I'll delete it. I also added a couple bullets explaining the blue and red lines.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe slightly better, still not a fan though. Maybe let some other folks look at it and see if they understand?

I'm slightly biased, but I like how the algorithm was displayed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe slightly better, still not a fan though. Maybe let some other folks look at it and see if they understand?

I'm slightly biased, but I like how the algorithm was displayed here.

I had a hard time following that, it struck me as a diagram written by someone very familiar with the algorithm. At any rate the explanation and diagrams belong in dotnet/docs#30426 which @IEvangelist owns, so he can own that.

IMAO, rate limiting is a gamechanger so I'm going to merge this so it can be live. I'll tweat it with a challenge to come up with a better diagram.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2022-08-30 13 39 11
@BrennanConroy I tried the preceding prototype but it looked worse.

@Rick-Anderson Rick-Anderson merged commit 701ea7d into main Aug 30, 2022
@Rick-Anderson Rick-Anderson deleted the rate-limit/ra branch August 30, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants