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

Retry get pull request calls to github #1131

Merged
merged 1 commit into from
Jul 26, 2020
Merged

Retry get pull request calls to github #1131

merged 1 commit into from
Jul 26, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Jul 26, 2020

These calls have been 404'ing lately, likely due to eventual consistency
issues on GitHub's side where they send the PR created webhook but it's
not yet available on their API.

To work around this, we will retry up to 3 times with a 1s delay.

Fixes #1019

These calls have been 404'ing lately, likely due to eventual consistency
issues on GitHub's side where they send the PR created webhook but it's
not yet available on their API.

To work around this, we will retry up to 3 times with a 1s delay.
@codecov
Copy link

codecov bot commented Jul 26, 2020

Codecov Report

Merging #1131 into master will increase coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1131   +/-   ##
=======================================
  Coverage   71.17%   71.17%           
=======================================
  Files          71       71           
  Lines        5876     5887   +11     
=======================================
+ Hits         4182     4190    +8     
- Misses       1346     1348    +2     
- Partials      348      349    +1     
Impacted Files Coverage Δ
server/events/vcs/github_client.go 86.69% <83.33%> (-0.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e04c454...67b2217. Read the comment docs.

@lkysow lkysow merged commit 0a1482d into master Jul 26, 2020
@lkysow lkysow deleted the retry-github branch July 26, 2020 17:24
// They've got some eventual consistency issues going on so we're just going
// to retry up to 3 times with a 1s sleep.
numRetries := 3
retryDelay := 1 * time.Second

Choose a reason for hiding this comment

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

Thank you for implementing this retries! 🤟

could you also please implement exponential backoff to space out the retries?

  1. on first retry you sleep for 1 sec
  2. on second retry you sleep for 2 sec
  3. on third retry you sleep for 4 sec.

jamengual pushed a commit that referenced this pull request Oct 4, 2021
* Improve github pull request call retries

Retry with fixed 1 second backoff up to 3 retries was added by #1131 to
address #1019, but the issue continued to show up (#1453).

Increase max attempts to 5 and use exponential backoff for a maximum
total retry time of (2^n - n - 1) seconds, which is roughly 30 seconds
for current max attempts n = 5.

Also move the sleep to the top of the loop so that we never sleep
without sending the request again on the last iteration.

* Fix style with gofmt -s
iainlane added a commit to iainlane/atlantis that referenced this pull request Jan 17, 2022
Similar to runatlantis#1131, we see this for /files/ too, resulting in a plan
error.

Closes runatlantis#1905
iainlane added a commit to iainlane/atlantis that referenced this pull request Jan 17, 2022
Similar to runatlantis#1131, we see this for /files/ too, resulting in a plan
error.

Closes runatlantis#1905
elementalvoid added a commit to elementalvoid/atlantis that referenced this pull request Jan 21, 2022
This is a follow on to resolve similar issues to runatlantis#1019.

In runatlantis#1131 retries were added to GetPullRequest. And in runatlantis#1810 a backoff
was included.

However, those only resolve one potential request at the very
beginning of a PR creation. The other request that happens early on
during auto-plan is one to ListFiles to detect the modified files. This
too can sometimes result in a 404 due to async updates on the GitHub
side.
chenrui333 pushed a commit that referenced this pull request Jan 25, 2022
Similar to #1131, we see this for /files/ too, resulting in a plan
error.

Closes #1905
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
* Improve github pull request call retries

Retry with fixed 1 second backoff up to 3 retries was added by runatlantis#1131 to
address runatlantis#1019, but the issue continued to show up (runatlantis#1453).

Increase max attempts to 5 and use exponential backoff for a maximum
total retry time of (2^n - n - 1) seconds, which is roughly 30 seconds
for current max attempts n = 5.

Also move the sleep to the top of the loop so that we never sleep
without sending the request again on the last iteration.

* Fix style with gofmt -s
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
Similar to runatlantis#1131, we see this for /files/ too, resulting in a plan
error.

Closes runatlantis#1905
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.

intermittent 404 Not Found on /pulls github calls
2 participants