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 rate limiter to client #494

Closed
wants to merge 6 commits into from

Conversation

dbriemann
Copy link
Contributor

Add a simple per client rate limiting based on golang.org/x/time/rate.

@SVilgelm
Copy link
Contributor

@jeevatkm Could you please check this? it would be great to have integrated rate limiter

@janekolszak
Copy link

So it looks ok, what needs to be done to finish this? I can help.

@jeevatkm jeevatkm removed the run-build label Mar 5, 2023
@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2023

@dbriemann I'm sorry for the delayed attention on the PR. Your feature proposal make sense, do you mind preparing the PR for review with tests & validation build success?

@dbriemann
Copy link
Contributor Author

I am very busy these days but I can have a look in the near future what exactly there is left to do.

@jeevatkm
Copy link
Member

I am very busy these days but I can have a look in the near future what exactly there is left to do.

@dbriemann Thanks for getting back.

@dbriemann
Copy link
Contributor Author

dbriemann commented Mar 24, 2023

OK I found some time to update this today. It is a very simple rate limiter which returns an ErrRateLimitExceeded on an execute that exceeds the limit. There are no automatic retries. The user has to handle these accordingly.

Not sure how good the API is. Just give it a review.

@dbriemann dbriemann marked this pull request as ready for review March 24, 2023 20:01
@jeevatkm
Copy link
Member

@dbriemann My apologies for the late. I have thought about it. Adding an interface contract into Resty will provide options for the users to any rate limiter or their version instead of integrating the library golang.org/x/time/rate.

In this PR, you have used the Allow() method from the available library options; I think we can start with this one method as a contract.

Are you interested in taking it up? Please let me know.

@SVilgelm
Copy link
Contributor

has been merged in #715, can be closed

@jeevatkm
Copy link
Member

jeevatkm commented Oct 1, 2023

Of-course @SVilgelm

@jeevatkm jeevatkm closed this Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants