1164645 - taskcluster-client-go: add exponential backoff retries for http request failures

Status: RESOLVED FIXED

(Taskcluster :: General, defect)

Description

[Pete Moore [:pmoore][:pete]]

If http requests to service endpoints fail, we should retry with exponential backoff, ideally tunable in the library.

Comment 1

[Pete Moore [:pmoore][:pete]]

Attachment: Pull Request

Please note I've opened up the PR for feedback, even though work not complete, as I'm off on holiday for a week.

Still TODO:
1) Properly handle various http response codes (e.g. probably all 4** errors shouldn't be retried - need to work out which ones should/shouldn't).
2) Make backoff configurable (e.g. possible to disable, change intervals, etc). This initial version has reasonable defaults that can't be altered.

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8605483 [details] [review]
Pull Request

See comments... Obviously this is not done (see your own TODOs).
The approach looks good to me, so f+ for that, I'm not sure how to respond to f?
here but f+ is so much more positive :)

Enjoy, you vacation...

Comment 3

[Greg Arndt [:garndt]]

Comment on attachment 8605483 [details] [review]
Pull Request

Looks good from what I understood of it.  Would be nice to see some tests (like you mention in your TODO)

Comment 4

[Pete Moore [:pmoore][:pete]]

OK I've finished that work off now.

The client is now using github.com/tj/go-debug library (same as livelog) so that debugging can be controlled by appropriate setting of the DEBUG environment variable.

I've also adopted godep dependency management (github.com/tools/godep) which is the same as s3-copy-proxy is using. This is currently a preferred way of freezing dependencies in go, but has the downside of increasing the project size, since it imports all dependencies into the source tree...

I've added tests, and updated the build script to also run tests, also to check that all generated code is checked in.

I've applied all the recommendations from the earlier feedback in the Pull Request.

Comment 5

[Pete Moore [:pmoore][:pete]]

Update: I ditched godeps - it was buggy when trying to godep restore on travis. The restore operation should copy from the local Godeps/_workspace into the global GOPATH. It was trying to do git/hg updates.

https://travis-ci.org/taskcluster/taskcluster-client-go/jobs/63653691#L160

After reading through the open issues for godep on github, I didn't have a good feeling it is going to serve us well, so decided to ditch it.

https://github.com/tools/godep/issues/50

We might want to consider using some form of url rewriting service, such that we can include package names that embed versions into them. Or something else.

Comment 6

[Pete Moore [:pmoore][:pete]]

Jonas: please note, I left the stubbed http server in there since for the tests I needed to control the responses to be a sequence of 4xx / 5xx / 2xx responses for a single API call. I couldn't really do that easily against the production environment. Since this patch was just about exponential backoff, I haven't added other tests for other topics, but when I do that, I'll test against production environment (with fake provisioner etc) wherever possible...

Comment 7

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8605483 [details] [review]
Pull Request

Minor comments on the PR, you can look at it and see if you want to address them, I don't think any of them are critical. The code seems solid to me.

Comment 8

[Pete Moore [:pmoore][:pete]]

Resetting Version and Target Milestone that accidentally got changed...