Remove overriding of retry handler. - #459
Conversation
Setting the retry handler to DEFAULT overrides the retry configuration, making it impossible to specify a default configuration. It seems to be unnecessary since allowing LoadBalancerContext to initialize it in initWithNiwsConfig appears to be sufficient for the case where there is no global configuration too.
|
Wonder if you could come up with a test? |
|
OK, so that opened a can of worms ;-) Secondly, my change did modify the existing behaviour. While RetryHandler.DEFAULT sets retryNextServer to 0, very unfortunately the value of DefaultClientConfigImpl.DEFAULT_MAX_AUTO_RETRIES_NEXT_SERVER, which is used to populate the client config, is set to 1, leading to 1 retry being made. I've had to alter the LBClientFactory to pass a new default configuration which uses 0 as the default for that setting. |
|
Does this look sensible now? I could remove the disabling of sun.net.http.retryPost in the test and increase the number of expected retries, if that'd be preferable, though I don't know what the outcome in other JVMs would be. |
| public static void disableSunRetry() throws Exception { | ||
| // The Sun HTTP Client retries all requests once on an IOException, which makes testing retry code harder than would | ||
| // be ideal. We can only disable it for post, so lets at least do that. | ||
| oldRetryConfig = System.setProperty(SUN_RETRY_PROPERTY, "false"); |
There was a problem hiding this comment.
wow didn't know about this one
There was a problem hiding this comment.
Neither did I until I wrote that test.
It's all about handling Keep-Alive connections. It'd be unfair on the application to propagate the IOException if it's caused by the reuse of a Keep-Alive connection that happened to be shutdown by the server (or a NAT/firewall timeout) at exactly the wrong moment, so the client will retry once when it looks like that might have happened (i.e. an IO error while writing a request or reading the response headers.)
This behaviour appears to be explicitly permitted, even for methods that aren't idempotent, by RFC7230 section 6.3.2.
Out of curiosity I had a quick dig through the Chromium source, and, assuming I'm reading it correctly, it too follows this behaviour. The Apache commons client, meanwhile, I think wont for a POST by default, which is why it'll sometimes throw inexplicable connection errors if keepalives are in use.
|
@ryanjbaxter you cool with this? |
|
So if I am understanding this correctly, if we don't specify a |
|
Basically, yes, since I felt I should preserve the default behaviour prior to this PR. The RetryHandler will now be instantiated using the ClientConfig, which as an instance of DisableAutoRetriesByDefaultClientConfig defaults to the same behaviour as RetryHandler.DEFAULT (i.e. no retries.) This allows the Ribbon retry behaviour to be optionally configured via the ConfigurationManager instance, overriding the default. |
|
👍 sounds good to me |
|
Just to clarify something regarding which retry logic we are talking about here... This change effect the Ribbon (ie. |
|
Yes, this only affects the Ribbon retry logic. |
|
I can merge this (or someone else can) tomorrow. Waiting a day in case
anyone changes their mind, as this is behavior affecting.
|
* Remove overriding of retry handler. Setting the retry handler to DEFAULT overrides the retry configuration, making it impossible to specify a default configuration. It seems to be unnecessary since allowing LoadBalancerContext to initialize it in initWithNiwsConfig appears to be sufficient for the case where there is no global configuration too. * Match previous retry behaviour, and add tests.
* Remove overriding of retry handler. Setting the retry handler to DEFAULT overrides the retry configuration, making it impossible to specify a default configuration. It seems to be unnecessary since allowing LoadBalancerContext to initialize it in initWithNiwsConfig appears to be sufficient for the case where there is no global configuration too. * Match previous retry behaviour, and add tests.
4 participants
Setting the retry handler explicitly to the DEFAULT instance overrides
the retry configuration, making it impossible to specify a configuration.
It seems to be unnecessary since allowing LoadBalancerContext to
initialize it in initWithNiwsConfig appears to be sufficient, as if
there's no configuration the defaults from there should be appropriate.