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 basic rate limit checker #706

Merged
merged 4 commits into from
Feb 21, 2020
Merged

Conversation

bitwiseman
Copy link
Member

This is a stripped down rate limit checking implementation that handles the infrastructure
of deciding how to get rate limit information and leaves it to other implementers to
decided what kind of checks they want to do and how long they want to wait.

The implementation supports checkers that sleep less than the full time until the
rate limit is expected to reset, allowing for polling and notifying clients of why their query
is not returning.

A basic checker which sleeps until the rate limit is expected to reset is included as working example..

Description

** Describe your change here**

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn -D enable-ci clean install site locally. This may reformat your code, commit those changes. If this command doesn't succeed, your change will not pass CI.

This is a stripped down rate limit checking implmentation that handles the infrastructure
of deciding how to get rate limit information and leaves it to other implementers to
decided what kind of checks they want to do and how long they want to wait.

The implmentation supports checkers that sleep less than the full time until the
rate limit is expected to reset, allowing for polling and notifying clients of why their query
is not returning.

A basic checker which sleeps until the rate limit is expected to reset is included as working example..
@bitwiseman bitwiseman force-pushed the task/guard branch 2 times, most recently from e9e690e to 3affedd Compare February 21, 2020 02:50
Whenever I submit a PR and then start looking at it as a reviewer, I immediately find a bunch of things that need changing.
Copy link
Contributor

@kshultzCB kshultzCB left a comment

Choose a reason for hiding this comment

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

LGTM. Well done and thanks for documenting stuff!

Comment on lines +54 to +56
// // 4897 is just the what the limit was when the snapshot was taken
// previousLimit = GHRateLimit
// .fromHeaderRecord(new GHRateLimit.Record(5000, 4897, System.currentTimeMillis() / 1000L));
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this can be removed entirely.

@bitwiseman bitwiseman merged commit 064206f into hub4j:master Feb 21, 2020
@bitwiseman bitwiseman deleted the task/guard branch February 21, 2020 21:06
* {@link RateLimitHandler#onError(IOException, HttpURLConnection)} will be called.
* </p>
* <p>
* NOTE: GitHub treats clients that exceed their rate limit very harshly. If possible, clients should avoid

Choose a reason for hiding this comment

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

Is there an official source out there that expresses this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

So then "GitHub treats clients ... harshly" refers to the further abuse rate limit apart from the regular rate limit?

My understanding is that the abuse rate limit is intended to limit something entirely different (too fast/concurrent calls/expensive calls vs exceeding total allowed calls per minute/hour).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, it is this:
https://docs.github.com/en/rest/guides/best-practices-for-integrators#dealing-with-rate-limits

If you hit a rate limit, it's expected that you back off from making requests and try again later when you're permitted to do so. Failure to do so may result in the banning of your app.

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 this pull request may close these issues.

4 participants