Retry commit loading when pushed dates are missing #500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Back in 1d06dfa, I removed retries for pushed date loading, preferring to always use a fallback API. This seemed to work better in most cases, but we're still seeing persistent but temporary failures loading pushed dates for certain merge commits. Add retries for the whole process again, covering both the initial pull request API and the fallback commit APIs, in the hope that this gives enough time for GitHub to update the internal data.
bluekeyes
commented
Nov 18, 2022
| // Retry with exponential backoff until it works or we hit the max total | ||
| // latency to avoid users thinking the bot got stuck or dropped an event. | ||
| const baseDelay = 1 * time.Second | ||
| const maxLatency = 20 * time.Second |
There was a problem hiding this comment.
I think doing a time-bounded retry is a bit easier to think about than a retry count, but open to switching this to a more traditional N attempts where we manually compute the expected max latency.
There was a problem hiding this comment.
Is there a way we can add loggging to make it clear that our worker thread is in a long sleep? If for whatever reason we've failed a few times in a row, its hard to know why its not doing anything from the logs.
ashmeer7
reviewed
Dec 1, 2022
| } | ||
| } | ||
|
|
||
| func (ghc *GitHubContext) loadCommitsOnce() (head *Commit, commits []*Commit, err error) { |
There was a problem hiding this comment.
I'll assume that this is only called one function above so we can have it do whatever.
ashmeer7
reviewed
Dec 1, 2022
| break | ||
| } | ||
| log.Debug().Dur("delay", delay).Str("sha", ghc.pr.HeadRefOID).Msg("Head commit is missing pushed data, sleeping and trying again") | ||
| time.Sleep(delay) |
There was a problem hiding this comment.
This message was going to be my suggestion for the above, so I think I'm happy.
ashmeer7
approved these changes
Dec 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Back in 1d06dfa, I removed retries for pushed date loading, preferring to always use a fallback API. This seemed to work better in most cases, but we're still seeing persistent but temporary failures loading pushed dates for certain merge commits.
Add retries for the whole process again, covering both the initial pull request API and the fallback commit APIs, in the hope that this gives enough time for GitHub to update the internal data.