WIP exponential backoff for node/go clients #89
Conversation
| @@ -13,6 +13,28 @@ function serializeQueryString(data) { | |||
| return data; | |||
| } | |||
|
|
|||
| const defaultRetryPolicy = { | |||
There was a problem hiding this comment.
I think we want to export this right?
There was a problem hiding this comment.
module.exports.RetryPolicies = {defaultRetryPolicy};?
| } | ||
| resolver(body); | ||
| }); | ||
| const retryPolicy = this.retryPolicy || options.retryPolicy || defaultRetryPolicy; |
There was a problem hiding this comment.
Can I pass null as a retryPolicy? I feel like in the current model I can't.
There was a problem hiding this comment.
But that is something I would assume I'd be able to do.
There was a problem hiding this comment.
I'm assuming by null you're trying to communicate "no retries"? If so, I think I'd rather export a NoRetryPolicy that you can pass (this is what I do with the Go client)
| let requestCount = 0; | ||
| let requestTimes = []; | ||
| const scope = nock("http://localhost:8000") | ||
| .get("/v1/books") |
There was a problem hiding this comment.
indents look a little funky here.
| @@ -0,0 +1,35 @@ | |||
| const assert = require("assert"); | |||
There was a problem hiding this comment.
These tests look real nice - can we make them run as part of make test in the wag repo?
There was a problem hiding this comment.
yeah, will set that up once this is all auto-generated. Right now it'll all get overwritten in CI
There was a problem hiding this comment.
we're still thinking of writing some tests where we don't mock the server along with these, right?
There was a problem hiding this comment.
yeah, I think we should eventually write some full E2E tests, but I'm not sure the best way to structure them...
|
|
||
| module.exports.RetryPolicies = { | ||
| Default: defaultRetryPolicy, | ||
| No: noRetryPolicy, |
| if r.Method == "POST" || r.Method == "PATCH" { | ||
| return resp, err | ||
| // Retry will retry non-POST, non-PATCH requests that 5XX. | ||
| // TODO: It does not currently retry any errors returned by net/http.Client's `Do`. |
There was a problem hiding this comment.
fyi, here's the Go transport retrying I mentioned https://github.com/golang/go/blob/master/src/net/http/transport.go#L403-L450
There was a problem hiding this comment.
interesting that they only retry GET, HEAD, OPTIONS, TRACE https://github.com/golang/go/blob/117c9c35cde8444d26db91bfa8346dde252989b1/src/net/http/request.go#L1252-L1260
There was a problem hiding this comment.
the rest of the retry logic there is pretty cryptic to me... do you understand what it's doing?
There was a problem hiding this comment.
only kind of. i'm still digging through the Go http library. will let you know if i understand it better as i read more
| @@ -93,7 +95,10 @@ func TestDefaultClientRetries(t *testing.T) { | |||
| c := client.New(testServer.URL) | |||
| _, err := c.GetBooks(context.Background(), &models.GetBooksInput{}) | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, 2, controller.getCount) | |||
| backoff := controller.getTimes[1].Sub(controller.getTimes[0]) | |||
| if backoff < 90*time.Millisecond || backoff > 110*time.Millisecond { | |||
There was a problem hiding this comment.
i recently learned about https://github.com/stretchr/testify/blob/master/assert/assertions.go#L722-L727 which makes this assert a little nicer
| @@ -93,7 +95,10 @@ func TestDefaultClientRetries(t *testing.T) { | |||
| c := client.New(testServer.URL) | |||
| _, err := c.GetBooks(context.Background(), &models.GetBooksInput{}) | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, 2, controller.getCount) | |||
| backoff := controller.getTimes[1].Sub(controller.getTimes[0]) | |||
There was a problem hiding this comment.
should we add a test for more than one retry?
| @@ -0,0 +1,35 @@ | |||
| const assert = require("assert"); | |||
There was a problem hiding this comment.
we're still thinking of writing some tests where we don't mock the server along with these, right?
rgarcia
force-pushed
the
exponential-backoff
branch
3 times, most recently
from
1298537
to
d37c7bc
Compare
rgarcia
force-pushed
the
exponential-backoff
branch
2 times, most recently
from
cb475d7
to
7327252
Compare
|
@cozmo finished auto generating everything and moved the js tests to their own directory--want to give it a final review? |
| @@ -0,0 +1,5 @@ | |||
| ca = null | |||
| //registry.npmjs.org/:_authToken=${npm_auth_token} | |||
There was a problem hiding this comment.
Only minor comment is I'd love to s/npm_auth_token/npm_token/ to be more consistent with the npm changes I made to the 60 repos over the last week.
|
@rgarcia Left one minor comment but otherwise LGTM. |
rgarcia
force-pushed
the
exponential-backoff
branch
from
7327252
to
929be01
Compare
also first node client test
currently this branch just contains direct modifications to the generated client code. I will autogenerate the code after I get the 馃憤 from @cozmo and @kvigen.