Add retries when loading config #887
Conversation
|
Thanks for your interest in palantir/policy-bot, @nikola-jokic! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
server/handler/fetcher.go
Outdated
| fc.ParseError = err | ||
| } else { | ||
| fc.Config = &pc | ||
| ticker.Reset(delay) |
There was a problem hiding this comment.
Let's use time.After here instead of manipulating a ticker - I think it will be easier to understand and I don't think there are any performance concerns with allocating a new timer if we're about to sleep for multiple seconds. Go 1.23 also fixed the issue with timers not being GC'd until after they fire.
There was a problem hiding this comment.
Sounds good, I guess I got used to using ticker before 1.23
server/handler/fetcher.go
Outdated
| Path: c.Path, | ||
| } | ||
| if err != nil { | ||
| if !os.IsTimeout(err) { |
There was a problem hiding this comment.
I think it would be worth re-trying other errors too, for instance if GitHub returns a 500 error or something.
There was a problem hiding this comment.
For this one, I guess the change needs to be done in palantir/go-githubapp since getFileContents receives a response and only propagates the error. In order to check for 500, there must either be an error type to check for on the caller side, or some other way to propagate 500 error somehow.
There was a problem hiding this comment.
The go-github library returns an error for any non-successful status code, so we have two options here without changing the upstream:
- Retry on any error from loading the content and accept that maybe that will sometimes retry things that will still fail
- Use
errors.Asto test for github.ErrorResponse and then check the status code in the response
There was a problem hiding this comment.
Updated with error check.
lmkwyt
This PR adds a retry mechanism with exponential backoff when fetching config returns a timeout error.
Fixes #506