spring-cloud / spring-cloud-netflix

Integration with Netflix OSS components
http://cloud.spring.io/spring-cloud-netflix/
Apache License 2.0
4.87k stars 2.44k forks source link

HOWTO: override DefaultLoadBalancerRetryHandler for all ribbon clients #2199

Closed hodeh closed 7 years ago

hodeh commented 7 years ago

I would like to override DefaultLoadBalancerRetryHandler in order to remove the SocketTimeOut exception from the retriable errors for all ribbon clients including the default , I am using Dalston.SR2 where the default retry handler is instantiated in the RibbonClientConfiguration. What I am unsure about is the proper way of setting the IClientConfig bean in the custom configuration class. i tried the following and it is currently complaining about the bean of IClientConfig

@Configuration
public class RibbonHAConfig {
    @Bean
    @Primary
    public RetryHandler retryHandler(IClientConfig config) {
        return new CustomHaRetryHandler(config);
    }
}
spencergibb commented 7 years ago

Follow the pattern described here http://cloud.spring.io/spring-cloud-static/Dalston.SR2/#_customizing_the_ribbon_client

hodeh commented 7 years ago

@spencergibb I tried to use this


@RibbonClients(defaultConfiguration = RibbonHAConfig.class)
@Configuration
public class RibbonHAConfig {

    @Bean
    public RetryHandler retryHandler(IClientConfig config) {
        return new CustomHaRetryHandler(config);
    }
}

but it is always complaining about a required a bean of type 'com.netflix.client.config.IClientConfig' that could not be found.

ryanjbaxter commented 7 years ago

Take a look at this sample and see if it helps https://github.com/spring-cloud-samples/tests/tree/master/ribbon-default-config. If you are still having issues maybe you can provide a sample demonstrating the issue.

hodeh commented 7 years ago

@ryanjbaxter thanks that comment in the test project you shared solved the problem

// Exclude @Configuration classes that should be included only in sub contexts created
// by Ribbon's SpringClientFactory. 

the bad news is that AbstractLoadBalancerAwareClient.isRetriableException is not being called and marked as deprecated , it seems it is not being used anymore ? so the custom retryhandler is never being called

ryanjbaxter commented 7 years ago

I was curious as to what you were doing because we no longer use the retry handler by default. If you are trying to configure Ribbon so it retries failed requests see http://cloud.spring.io/spring-cloud-static/Dalston.SR2/#retrying-failed-requests.

hodeh commented 7 years ago

I enabled retry and by default Zuul was retrying on next servers if a read time out happened, this is a case where I don't want to retry requests after , I want retrials to be done on on HostConnectExceptions ,So I thought that by overriding the DefaultLoadBalancerRetryHandler which declares the SocketTimeout exceptions would solve the problem for me, but as you said it seems RetryHandleris not being used by default, So I am wondering why it is currently retrying on time outs, and would i configure it to retry exclusively on HostConnectException GIVEN that using the property "retryableStatusCodes" wont solve the problem because the server i am sending the request to won't return a status code at all in case of HostConnectException

ryanjbaxter commented 7 years ago

Are you talking about this exception? https://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/conn/HttpHostConnectException.html

hodeh commented 7 years ago

java.net.ConnectException is the only exception i would it to be retried on next severs. i want to exclude the SocketTimeoutException from being retried

ryanjbaxter commented 7 years ago

You will probably have to to implement your own LoadBalancedRetryPolicyFactory to return a custom RetryPolicy which only retries on certain exceptions. SimpleRetryPolicy from Spring Retry does something like this. https://github.com/spring-projects/spring-retry/blob/b6aadb0f13cc2f48b61765c0f4dec722516e4057/src/main/java/org/springframework/retry/policy/SimpleRetryPolicy.java

You will have to combine that logic with what we already do in RibbonLoadBalancedRetryPolicy.

hodeh commented 7 years ago

Thanks a million, that worked for me a custom LoadBalancedRetryPolicyFactory, that builds a custom LoadBalancedRetryPolicy which extends a RibbonLoadBalancedRetryPolicy with an overriden canRetry()