Skip to content

Fix #775: Wrong rate limit returned for search #776

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
Merged
merged 5 commits into from
Jan 30, 2018
Merged

Fix #775: Wrong rate limit returned for search #776

merged 5 commits into from
Jan 30, 2018

Conversation

justfortherec
Copy link
Contributor

@justfortherec justfortherec commented Jan 26, 2018 •
edited
Loading

This pull request is an attempt to fix issue #775

@justfortherec
Fix sigmavirus24#775: Wrong rate limit returned for search
fb983cb 
This change adds a field which indicates against what rate limit API
calls from an object are counted against.

GitHub API v3 enforces different rate limits [1] for subsets of API
calls. The `search` API has custom rate limts.[2]

`GitHubCore` has a property `remaining_ratelimit` which requests rate
limit information from endpoint `GET /rate_limit` [3] and caches the
number in `GitHubCore._remaining`.

The API call returns information about all rate limits but previously
`GitHubCore.remaining_ratelimit` only exposed the one for the `core`
resources.

With this change the resource still defaults to `core` but objects
sending search request change this to `search`.

[1]: https://developer.github.com/v3/#rate-limiting
[2]: https://developer.github.com/v3/search/#rate-limit
[3]: https://developer.github.com/v3/rate_limit/
@justfortherec
Make SearchIterator return rate limits for search
246b825
sigmavirus24
sigmavirus24 reviewed Jan 26, 2018
View reviewed changes
github3/models.py Outdated
@@ -42,6 +42,7 @@ def __init__(self, json, session=None):

# set a sane default
self._github_url = 'https://api.github.com'
self._ratelimit_resource = 'core'
Copy link
Owner

Choose a reason for hiding this comment

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

Could this be a class attribute, e.g.,

class GitHubCore(...):
    _ratelimit_resource = 'core'

Then we could have

class GitHubSearchBase(GitHubCore):
    """..."""

    _ratelimit_resource = 'search'

without having to override __init__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@justfortherec
Copy link
Contributor Author

I am not quite sure the changes to search result classes (in search/) are even necessary. They don't seem to have any methods that would incur rate limit other than refresh(). And that counts against the core rate limit.

Are there any other methods that I would need to take into account?

Is I see it at the moment, I'd rather remove the entire GitHubSearch class and only have the SearchIterator return the search rate limit. What do you think?

@sigmavirus24
Copy link
Owner

Are there any other methods that I would need to take into account?

I don't think so.

I'd rather remove the entire GitHubSearch class and only have the SearchIterator return the search rate limit.

I guess the question for you and @omgjlk is: How often do you use the ratelimit_remaining attribute and can that whole thing be removed altogether?

@justfortherec
Copy link
Contributor Author

How often do you use the ratelimit_remaining attribute and can that whole thing be removed altogether?

Why would you remove the entire method? For search result classes it could/should still return the core rate limit, IMO. For instance calling issue_search_result.refresh() is counted against the core rate limit.

@justfortherec
Remove class GitHubSearch
b4d7ef8 
Requests sent from search result classes do not actually count against
the 'search' rate limit. Only search requests (i.e. by a
`SearchIterator` incur 'search' rate limit.
@omgjlk
Copy link
Collaborator

omgjlk commented Jan 29, 2018

We call github.rate_limit some 18~ times in zuul's GitHub driver, after just about every interaction with the API. However I see that calling that should return us either core or search, and we're not properly looking at the search results vs core results when logging that information. I'm going to change it there soon :) We don't appear to make any use of ratelimit_remaining directly, we just get it from the results of the rate_limit method.

@justfortherec
Copy link
Contributor Author

Is there anything you would want to like improved in order to to get this merged?

@sigmavirus24 sigmavirus24 merged commit 6e110c5 into sigmavirus24:develop Jan 30, 2018
@sigmavirus24
Copy link
Owner

Thanks for iterating on this with us @justfortherec!

@mtb-beta mtb-beta mentioned this pull request Dec 3, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sigmavirus24 sigmavirus24 sigmavirus24 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justfortherec @sigmavirus24 @omgjlk