Configurable client for faster timeouts #50
Conversation
client/client_test.go
Outdated
| } | ||
| apiClient := NewClient(clientConfig) | ||
| _, _, err := apiClient.GetUserOrganizations("foo", "foo", "foo", "foo") | ||
| if err == nil || !strings.Contains(err.Error(), "Client.Timeout exceeded") { |
There was a problem hiding this comment.
It would be good to not compare strings for error assertions. Isn't there an error type we can compare against?
There was a problem hiding this comment.
Unfortunately, the error type isn't specific enough: *url.Error.
There was a problem hiding this comment.
When it is a URL error, we can check if it is an timeout error. See https://golang.org/pkg/net/url/#Error.Timeout.
client/client_test.go
Outdated
|
|
||
| func TestTimeout(t *testing.T) { | ||
| clientConfig := Configuration{ | ||
| Endpoint: "http://httpbin.org/delay/2?", |
There was a problem hiding this comment.
This test now depends on the network and some remote service. When either of these fails, the test fails. An alternative would be to start a test server and sleep in its handler to simulate the delay. See https://golang.org/pkg/net/http/httptest/.
There was a problem hiding this comment.
I think depending on the network for tests should be fine. But I will look at the alternative. Having it totally internal is definitely better.
commands/create_cluster.go
Outdated
| responseBody, apiResponse, _ := client.AddCluster(authHeader, addClusterBody, requestIDHeader, createClusterActivityName, cmdLine) | ||
| clientConfig := client.Configuration{ | ||
| Endpoint: cmdAPIEndpoint, | ||
| Timeout: 60 * time.Second, |
There was a problem hiding this comment.
Would be cool to define a default timeout which we can use where needed.
| client := gsclientgen.NewDefaultApiWithBasePath(cmdAPIEndpoint) | ||
| clientConfig := client.Configuration{ | ||
| Endpoint: cmdAPIEndpoint, | ||
| Timeout: 3 * time.Second, |
There was a problem hiding this comment.
What is actually the reasoning behind the very different timeouts?
There was a problem hiding this comment.
I want to fail fast in situations where multiple GET calls are made in sequence. E. g. when listing clusters, we first fetch all the organizations (memberships). Then we look for clusters in each org. This failing after one minute (as it happened just recently with flapping API behaviour) is just a really bad waste of time for the end user. I'd rather fail after a few seconds and let the user retry.
In cases of transactions (POST, PUT, PATCH) I'd like to tolerate longer timeouts, as we expect a little bit more work to happen on the other end, and retrying might not be as easy (unless transactions are implemented for that route).
The next step of course will be doing the retrying automatically, so the user doesn't have to bother at all in most cases.
It's a tiny step, but I see it as a little piece in a bigger resiliance picture.
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 28.42% 30.36% +1.93%
==========================================
Files 22 23 +1
Lines 781 830 +49
==========================================
+ Hits 222 252 +30
- Misses 535 554 +19
Partials 24 24
Continue to review full report at Codecov.
|
|
@xh3b4sd Thanks for the review! Happy for a re-check. |
| // enforce a timeout longer than the client's | ||
| time.Sleep(3 * time.Second) | ||
| fmt.Fprintln(w, "Hello") | ||
| })) |
There was a problem hiding this comment.
I like that way more than before. Cool.
|
Changed error type checking. |
This PR adds a way to configure the API client for
This is applied for all API requests.
The motivation were lots of timeouts when using our current leaseweb-g8s backend, especially in commands like
list clusters, where multiple requests are made.Technically this could have been part of
gsclientgen, but in fact I don't want to add code there, as all code in that repository is generated by swagger.