PCI-3118 rate limit retry

Pix4D / cogito

Concourse resource for GitHub Commit Status and Google Chat notifications

MIT License

35 stars 14 forks source link

PCI-3118 rate limit retry #118

Closed aliculPix4D closed 1 year ago

aliculPix4D commented 1 year ago

Commit 1: (NO CHANGE IN THE BEHAVIOUR) github: refactor code to allow adding retry logic

collect and store response status code, body and some header info (about oauth and rate limit)

Commit 2: github: implement retry mechanism when rate limited

Just a draft for now to collect a bit of feedback...

If desired, I can already activate retry also for status codes >500 in this PR

aliculPix4D commented 1 year ago

@Pix4D/integration I will put the PR on hold until I get the comments from @marco-m

marco-m-pix4d commented 1 year ago

@aliculPix4D I left some inline comments.

Regarding the approach I mentioned at the standup, it would involve injecting in the HTTP Client a custom Roundtripper (and this RoundTripper would take care of the retry). That to me would be the correct approach, on the other hand it is not as immediate as I thought, so it might be more pragmatic to use the current approach.

If I look at the code in the PR, my first impression is that it should be possible to simplify it, but I cannot put the finger on where.

aliculPix4D commented 1 year ago

@marco-m-pix4d I addressed all the comments and give you my opinion on the value of MaxSleepTime variable.

aliculPix4D commented 1 year ago

Note that we need to mention this behavior (retry in case of rate limit) both in the README and CHANGELOG.

Yes, will do that in another PR...