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.

Sign up for GitHub

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

Jump to bottom

Implement rate limit handling #238

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Implement rate limit handling #238

wants to merge 1 commit into from

Conversation

rickardl
Copy link
Contributor

Implements a rate-limit function to support #233 and resolve #237

func rateLimited(err error) bool {
	if err == nil {
		return false
	}
	rerr, ok := err.(*github.RateLimitError)
	if ok {
		var d = rerr.Rate.Reset.Time.Sub(time.Now())
		//hit rate limit, sleeping
		time.Sleep(d)
		return true
	}
	aerr, ok := err.(*github.AbuseRateLimitError)
	if ok {
		var d = aerr.GetRetryAfter()
		//hit abuse mechanism, sleeping
		time.Sleep(d)
		return true
	}
	return false
}

implemented as

for {
    err := APIRequest()
    if err != nil {
        if rateLimited(err) {
            continue
        } else {
            return err
        }
    }
}

in related GitHub v3 API requests.

@rickardl
Copy link
Contributor Author

rickardl commented Oct 22, 2020 •
edited
Loading

@itsdalmo I might be a bit pre-emptive in my implementation here, I'll see if we can mock and test this behavior later, do you have an idea on how to proceed?

@rickardl rickardl self-assigned this Oct 22, 2020
@d
Copy link

d commented Oct 22, 2020

OMG, we just hit this today, 👍 for posting this patch!

@rickardl
Copy link
Contributor Author

rickardl commented Oct 23, 2020 •
edited
Loading

@d Please lock down to v0.21.0 until this gets carefully reviewed and merged :-)

d added a commit to d/gpdb that referenced this pull request Oct 23, 2020
@d
Pin PR resource to v0.21 to avoid Github abuse rate limit
9b936bc 
We started hitting this on Thursday, and there's been ongoing report
from the community about this as well. While upstream is figuring out a
long term solution [1], we've been advised [2] to pin to the previous
release (v0.21.0) to avoid being blocked for hours at once.

[1]: telia-oss/github-pr-resource#238
[2]: telia-oss/github-pr-resource#238 (comment)
@itsdalmo
Copy link
Contributor

I think the change here looks reasonable, but considering that we are hitting the abuse rate limit I take that as an indication that we are doing something we should not be doing here. Looking at the diff between v0.21.0 and v0.22.0 I noticed this line. I.e., when fetching pull requests we no longer filter out only those that are open but instead we always fetch everything, which for repositories with a lot of history and activity will always result in hitting a rate limit no matter how many times we retry.

As such, I think think the best fix here is to either roll back the change I linked above or to do the filtering in the GraphQL query (instead of fetching everything and then filtering in the resource as is currently done). And I suggest we try that before implementing a silent retry for rate limit errors 🤔

@jmccann
Copy link

jmccann commented Oct 27, 2020

While working on updating #189 and testing it I ran into this exact issue and @itsdalmo hit the nail on the head. Whereas before on initial fetch of PRs to filter on we only queried for OPEN ones we now query for basically ALL of them. This resulted in a check that takes a long time to run. I've been doing manual testing but am sure if I had it running every minute I'd for sure have hit the rate limit ... which doesn't reset for an hour I think.

@jmccann jmccann mentioned this pull request Oct 27, 2020
Closed
kalensk pushed a commit to kalensk/gpdb that referenced this pull request Oct 27, 2020
@d @kalensk
Pin PR resource to v0.21 to avoid Github abuse rate limit
cdc683b 
We started hitting this on Thursday, and there's been ongoing report
from the community about this as well. While upstream is figuring out a
long term solution [1], we've been advised [2] to pin to the previous
release (v0.21.0) to avoid being blocked for hours at once.

[1]: telia-oss/github-pr-resource#238
[2]: telia-oss/github-pr-resource#238 (comment)

(cherry picked from commit f4bf9be)
kalensk added a commit to kalensk/gpdb that referenced this pull request Oct 27, 2020
@kalensk @d
Pin PR resource to v0.21 to avoid Github abuse rate limit
b7e8ee2 
We started hitting this on Thursday, and there's been ongoing report
from the community about this as well. While upstream is figuring out a
long term solution [1], we've been advised [2] to pin to the previous
release (v0.21.0) to avoid being blocked for hours at once.

[1]: telia-oss/github-pr-resource#238
[2]: telia-oss/github-pr-resource#238 (comment)

Co-authored-by: Jesse Zhang <[email protected]>
@bgandon bgandon mentioned this pull request May 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rickardl rickardl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement support for rate limits from GitHub
4 participants
@rickardl @d @itsdalmo @jmccann