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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add an option to wait for primary rate limit reset and retry instead of erroring out #3114

Closed
erezrokah opened this issue Mar 28, 2024 · 7 comments 路 Fixed by #3117
Closed

Comments

@erezrokah
Copy link
Contributor

Hi 馃憢 馃惏

This is mostly the same issue as #1920

For our use case we'd prefer to wait until the reset time instead of erroring out when the primary rate limit is reached.
The only way I found to do it at the moment is wrap each API call with a check to github.RateLimitError.

We can't use roundtripper as checkRateLimitBeforeDo happens before. I tried wrapping BareDo with my own implementation but couldn't get it to work due to how the services and client are initialized.

If bypassRateLimitCheck was exported I might have been able to use it to bypass the check and get use a roundtripper but that doesn't seem very elegant.

Can you suggestion a solution?

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 28, 2024

What did you think of my earlier suggestion?

@erezrokah
Copy link
Contributor Author

That would work great for us

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 28, 2024

OK, cool. Would you like to submit a PR, @erezrokah , or would you prefer that I open this up to our other contributors?

@erezrokah
Copy link
Contributor Author

I can open a PR if you point me to the right direction. Is the suggestion a new pattern to the repo or an existing pattern there's a reference for?

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 28, 2024

I believe this would be new functionality. As such, I have not explored the solution space.
So if you don't have a plan in mind at this point, then maybe we should let another developer tackle this problem, as it will involve some investigation, some thought, and some experimentation to get it right.

@erezrokah
Copy link
Contributor Author

I'm ok with that no worries, wanted to make sure I'm not missing anything. I'll take a look and reach out if I'm blocked

@erezrokah
Copy link
Contributor Author

Opened a PR #3117, feedback welcomed

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 a pull request may close this issue.

2 participants