tap-github icon indicating copy to clipboard operation
tap-github copied to clipboard

handle rate limiter & github server-side errors

Open mredolatti opened this issue 5 years ago • 13 comments

Two issues were found while using this library:

  • Random and isolated 502 errors from github make the whole job fail.
  • If the number of API calls exceeds github's limit, the whole job fails.

To deal with these scenarios, simple retry logic was added, and in the case of a rate limiting situation, we wait until the next reset time to move forward with the next retry.

mredolatti avatar May 24 '19 20:05 mredolatti

Hi @mredolatti, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

cmerrick avatar May 24 '19 20:05 cmerrick

You did it @mredolatti!

Thank you for signing the Singer Contribution License Agreement.

cmerrick avatar May 24 '19 20:05 cmerrick

Are there plans to work on this? @osterman or @mredolatti ? I am having the same issue as well

nehiljain avatar Jul 07 '20 00:07 nehiljain

@cmerrick @mredolatti any way we can help to get this over the line?

henriblancke avatar Jul 07 '20 17:07 henriblancke

@nehiljain Sorry - no resources available on outside to put towards it at this time

osterman avatar Jul 07 '20 19:07 osterman

@osterman @mredolatti @luandy64, what needs to happen to merge this PR? It looks like the code that's there already is much better than the current code, which has no retry capability. Also, it doesn't look like backoff will handle the rate limit headers anyway.

This issue is blocking me from using Stitch's hosted Github tap, and instead I have to run it on my own.

KBorders01 avatar Jan 03 '21 17:01 KBorders01

Up! Can we merge this?

antoine-lizee avatar Oct 06 '21 10:10 antoine-lizee

@luandy64 can we merge this PR or is has it been implemented elsewhere? Hitting the GitHub ratelimit is affection our ability to use Stitch as well

savicbo avatar May 01 '22 03:05 savicbo

is this still an issue? i took a look at backoff a while ago and didn't see a straightforward way to use it while relying on the response's headers to actually wait the correct amount of time. Do we need to add that behavior into backoff first and then update this?

mredolatti avatar May 03 '22 02:05 mredolatti

@mredolatti latest backoff (2.0.1) has the backoff.runtime decorator which allows you to introspect the exception or response and yield a wait value based on that

bgreen-litl avatar May 05 '22 13:05 bgreen-litl

@mredolatti @bgreen-litl basic backoff functionality was added in #143. This PR should probably be closed and a custom wait time functionality can be added in a new PR based on the latest version in master.

loeakaodas avatar May 05 '22 21:05 loeakaodas

@savicbo @mredolatti @bgreen-litl @antoine-lizee @KBorders01 @henriblancke @nehiljain

Correct me if I'm wrong, but if you run v1.10.4 of this tap, there's already logic to handle the rate limit headers thanks to @loeakaodas https://github.com/singer-io/tap-github/blob/4f7ba58e501bd5026979194a0c25f31d32d3ffe0/tap_github/init.py#L198-L207

luandy64 avatar May 23 '22 14:05 luandy64

@savicbo @mredolatti @bgreen-litl @antoine-lizee @KBorders01 @henriblancke @nehiljain

Correct me if I'm wrong, but if you run v1.10.4 of this tap, there's already logic to handle the rate limit headers thanks to @loeakaodas

https://github.com/singer-io/tap-github/blob/4f7ba58e501bd5026979194a0c25f31d32d3ffe0/tap_github/init.py#L198-L207

@luandy64 that is correct, it looks like this issue has been resolved in another PR.

KBorders01 avatar May 23 '22 15:05 KBorders01