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 limit handling #600

Closed
Zlopez opened this issue Aug 30, 2018 · 17 comments
Closed

Add rate limit handling #600

Zlopez opened this issue Aug 30, 2018 · 17 comments
Assignees
Labels
Medium Priority This ticket has a medium priority type.feature New feature
Milestone

Comments

@Zlopez
Copy link
Contributor

Zlopez commented Aug 30, 2018

In current state GitHub API is raising RateLimit exception, which is only logged by cronjob.

It will be nice to reschedule the projects check when rate limit is reset. This could be handled by saving all the projects with GitHub backend after exception is raised and run check again only for them.

@Zlopez Zlopez added type.feature New feature Medium Priority This ticket has a medium priority labels Aug 30, 2018
@abitrolly
Copy link
Contributor

A better architecture is a proper queue/microservice with rate limiting, which flexibly adapts itself to rate limits, and exposes logs and exceptions to public.

But first, I would like to see stats.

@Zlopez
Copy link
Contributor Author

Zlopez commented Sep 3, 2018

Yes, this is something I have in my TODO. To better handle the amount of requests. Right now, we are doing check for all the projects in one go, which is not really nice.
I want to divide this load to the whole day. So it will check only a few projects every hour (maybe something more complex later).
The exposition of the logs and exceptions should be done using fedmsg like you proposed in #574
However, I'm not sure if there is any topic for this right now.

I don't understand the link you sent, this is pointing to tags sdispater/poetry project. How this has to do anything with stats?

@Zlopez Zlopez added this to the 0.14.0 milestone Sep 6, 2018
@Zlopez Zlopez self-assigned this Oct 8, 2018
@Zlopez Zlopez changed the title Add rate limit handling for GitHub API Add rate limit handling Oct 8, 2018
@Zlopez
Copy link
Contributor Author

Zlopez commented Oct 8, 2018

I change the title, because this is not only about GitHub.

@yarons
Copy link
Contributor

yarons commented Nov 21, 2018

I think that the most basic part is mapping the rate limit and availability and then trying to fit into this frame, and most importantly: when the first rejection occurs (rate limit) the system should freeze the checks instead of keeping on probing for new versions marking everything as rate limited.

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 21, 2018

@yarons
This is not a bad idea, but we have plenty of backends and it's not good to freeze everything, because one backend reaches rate limit.
In PR #633 I have it implemented for every project separately. Each project is checked after specified time (currently 1 hour) and if rate limit is thrown it is rescheduled to time returned in rate limit exception.

@yarons
Copy link
Contributor

yarons commented Nov 21, 2018

I meant freezing the specific backend, not all of them 😆.

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

I will add this to my TODO list and look at it later. For now it will work on per project basis.

@yarons
Copy link
Contributor

yarons commented Nov 22, 2018

So I should be expecting lots of failures ahead :)

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

I didn't noticed any ratelimit error with the current number of projects. So I don't think we will hit this anytime soon.

@yarons
Copy link
Contributor

yarons commented Nov 22, 2018

image
Hmmmm sure?

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

Oh, last time I checked this, there wasn't any rate limit error.

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

It now looks like we are hitting those, but the current PR should still help with this.

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

There was one thing I didn't noticed before, how the actual ratelimit cap is handled by GitHub. So I need to change how ratelimit is detected.

@yarons
Copy link
Contributor

yarons commented Nov 22, 2018

So in case of rate limit detection per backend it should stop querying once the first failed.

The only problem is that there is no indication so if we're almost at the limit and I'm trying to add another service then when I'll save it and ask for querying I will hit the limit while working with the system so we shouldn't stop at the actual limit but maybe using only 90% of it and keeping some buffer for contributors work.

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

This is probably good idea. To not waste whole rate limit at once.

@Zlopez
Copy link
Contributor Author

Zlopez commented Nov 22, 2018

I created a new issue, that contains the discussed rate limit enhancements #665. Feel free to comment on it.

@Zlopez
Copy link
Contributor Author

Zlopez commented Jan 10, 2019

This issue is already implemented by #678. Closing.

@Zlopez Zlopez closed this as completed Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Medium Priority This ticket has a medium priority type.feature New feature
Projects
None yet
Development

No branches or pull requests

3 participants