Change client behavior on 503 from retry to failover #686
Conversation
|
this looks good. i'll have to run for dinner now, but will take a more in-depth look later. |
|
Ugh looks like the build failed :( I’ll be able to take a look later tonight
…On Tue, Feb 27, 2018 at 17:52 Robert Fink ***@***.***> wrote:
this looks good. i'll have to run for dinner now, but will take a more
in-depth look later.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#686 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF4z26_7edwqx6iLkYL7c0uHZYPIHdbKks5tZLFogaJpZM4SV6tQ>
.
|
| @@ -118,10 +118,23 @@ public void testConnectionError_performsFailoverOnDnsFailure() throws Exception | |||
|
|
|||
| @Test | |||
| public void testQosError_performsFailover() throws Exception { | |||
| server1.enqueue(new MockResponse().setResponseCode(503)); | |||
| server1.enqueue(new MockResponse().setResponseCode(UNAVAILABLE_RESPONSE_CODE)); | |||
There was a problem hiding this comment.
think I prefer the 503 here
| @@ -118,10 +118,23 @@ public void testConnectionError_performsFailoverOnDnsFailure() throws Exception | |||
|
|
|||
| @Test | |||
| public void testQosError_performsFailover() throws Exception { | |||
| server1.enqueue(new MockResponse().setResponseCode(503)); | |||
| server1.enqueue(new MockResponse().setResponseCode(UNAVAILABLE_RESPONSE_CODE)); | |||
There was a problem hiding this comment.
think I prefer the 503 here
There was a problem hiding this comment.
can you add a similar test for okhttp-clients?
| scheduleExecution(callback, backoff.get()); | ||
| } | ||
| return null; | ||
| return redirectToUrl(callback, call, |
There was a problem hiding this comment.
I'd like to retry another host with backoff
There was a problem hiding this comment.
Are you thinking of the single-node case here? I thought about this and ended up going this way just to index on the multi-node case - if we get a 503 from one node, we have no data on the health of the other nodes, thus we can safely assume (as we always do when we are issuing the first try of a request) that any other node is healthy right now, and we can try immediately with no backoff. But in the single-node case we'd definitely want to back off. But the logic of how many URLs there are shouldn't be present at this level... and I guess you could run into the case where all nodes are unavailable due to load, and the client is getting bounced around, in which case it still makes sense to back off. And backing off the first time or two probably isn't the worst penalty to pay. Okay.
| @@ -212,10 +213,10 @@ public void onResponse(Call call, Response response) throws IOException { | |||
| } | |||
|
|
|||
| @SuppressWarnings("FutureReturnValueIgnored") | |||
| private void scheduleExecution(Callback callback, Duration backoff) { | |||
| private void scheduleExecution(Callback callback, Duration backoff, Runnable execution) { | |||
There was a problem hiding this comment.
prefer to order (execution, backoff)
| () -> urls.redirectToNext(request().url()), | ||
| "Failed to determine valid redirect URL for base URLs " + urls.getBaseUrls(), | ||
| request -> scheduleExecution(() -> client.newCallWithMutableState(request, | ||
| backoffStrategy, maxNumRelocations - 1).enqueue(callback), backoff.get())); |
There was a problem hiding this comment.
don't think we need to decrease maxNumRelocations in this case. backoff already limits the number of retries for 503s.
| } | ||
| return null; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private void redirectToUrl(Callback callback, Call call, Supplier<Optional<HttpUrl>> optionalUrlSupplier, |
There was a problem hiding this comment.
think this construction is pretty hard to grok, maybe be more readable with a little bit of code duplication in the visit() methods. summarizing:
- RetryOther should fail if maxNumRetries <= 0 or no URL can be determined
- Unavailable should fail if backoff is empty or no URL can be determined
There was a problem hiding this comment.
yeah realized i was possibly going overboard with the DRY there
| server2.enqueue(new MockResponse().setResponseCode(200).setBody("foo")); | ||
| public void handlesQos_failsOverToAnotherUrl() throws Exception { | ||
| server.enqueue(new MockResponse().setResponseCode(503)); | ||
| server.enqueue(new MockResponse().setBody("foo")); |
| server.enqueue(new MockResponse().setResponseCode(308).addHeader(HttpHeaders.LOCATION, url2)); | ||
| server2.enqueue(new MockResponse().setResponseCode(503)); | ||
| server2.enqueue(new MockResponse().setResponseCode(200).setBody("foo")); | ||
| public void handlesQos_failsOverToAnotherUrl() throws Exception { |
There was a problem hiding this comment.
handlesQos_503FailsOverToAnotherUrl
|
thanks, @gregakinman |
|
np! would like to cut 3.19.0 with this too if you weren't planning to already |
@uschi2000
Fixes #678