Skip to content

Commit

Permalink
Refactor spring retry code (#10)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ryanjbaxter @spencergibb
ryanjbaxter authored and spencergibb committed Mar 13, 2018
1 parent c45f153 commit 485583d
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 233 deletions.
59 changes: 6 additions & 53 deletions ...ain/java/org/springframework/cloud/openfeign/ribbon/CachingSpringLoadBalancerFactory.java
View file
Expand Up @@ -18,10 +18,7 @@

import java.util.Map;

import org.springframework.cloud.client.loadbalancer.LoadBalancedBackOffPolicyFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryListenerFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryPolicyFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory;
import org.springframework.cloud.netflix.ribbon.ServerIntrospector;
import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
import org.springframework.util.ConcurrentReferenceHashMap;
Expand All @@ -40,61 +37,17 @@
public class CachingSpringLoadBalancerFactory {

private final SpringClientFactory factory;
private final LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory;
private final LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory;
private final LoadBalancedRetryListenerFactory loadBalancedRetryListenerFactory;
private boolean enableRetry = false;
private LoadBalancedRetryFactory loadBalancedRetryFactory = null;

private volatile Map<String, FeignLoadBalancer> cache = new ConcurrentReferenceHashMap<>();

public CachingSpringLoadBalancerFactory(SpringClientFactory factory) {
this.factory = factory;
this.loadBalancedRetryPolicyFactory = new RibbonLoadBalancedRetryPolicyFactory(factory);
this.loadBalancedBackOffPolicyFactory = null;
this.loadBalancedRetryListenerFactory = null;
}

@Deprecated
//TODO remove in 2.0.x
public CachingSpringLoadBalancerFactory(SpringClientFactory factory,
LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory) {
public CachingSpringLoadBalancerFactory(SpringClientFactory factory, LoadBalancedRetryFactory loadBalancedRetryPolicyFactory) {
this.factory = factory;
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.loadBalancedBackOffPolicyFactory = null;
this.loadBalancedRetryListenerFactory = null;
}

@Deprecated
//TODO remove in 2.0.0x
public CachingSpringLoadBalancerFactory(SpringClientFactory factory,
LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory, boolean enableRetry) {
this.factory = factory;
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.enableRetry = enableRetry;
this.loadBalancedBackOffPolicyFactory = null;
this.loadBalancedRetryListenerFactory = null;
}

@Deprecated
//TODO remove in 2.0.0x
public CachingSpringLoadBalancerFactory(SpringClientFactory factory,
LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory,
LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory) {
this.factory = factory;
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.loadBalancedBackOffPolicyFactory = loadBalancedBackOffPolicyFactory;
this.loadBalancedRetryListenerFactory = null;
this.enableRetry = true;
}

public CachingSpringLoadBalancerFactory(SpringClientFactory factory, LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory,
LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory,
LoadBalancedRetryListenerFactory loadBalancedRetryListenerFactory) {
this.factory = factory;
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.loadBalancedBackOffPolicyFactory = loadBalancedBackOffPolicyFactory;
this.loadBalancedRetryListenerFactory = loadBalancedRetryListenerFactory;
this.enableRetry = true;
this.loadBalancedRetryFactory = loadBalancedRetryPolicyFactory;
}

public FeignLoadBalancer create(String clientName) {
Expand All @@ -105,8 +58,8 @@ public FeignLoadBalancer create(String clientName) {
IClientConfig config = this.factory.getClientConfig(clientName);
ILoadBalancer lb = this.factory.getLoadBalancer(clientName);
ServerIntrospector serverIntrospector = this.factory.getInstance(clientName, ServerIntrospector.class);
client = enableRetry ? new RetryableFeignLoadBalancer(lb, config, serverIntrospector,
loadBalancedRetryPolicyFactory, loadBalancedBackOffPolicyFactory, loadBalancedRetryListenerFactory) : new FeignLoadBalancer(lb, config, serverIntrospector);
client = loadBalancedRetryFactory != null ? new RetryableFeignLoadBalancer(lb, config, serverIntrospector,
loadBalancedRetryFactory) : new FeignLoadBalancer(lb, config, serverIntrospector);
this.cache.put(clientName, client);
return client;
}
Expand Down
10 changes: 3 additions & 7 deletions ...n/java/org/springframework/cloud/openfeign/ribbon/FeignRibbonClientAutoConfiguration.java
View file
Expand Up @@ -21,9 +21,7 @@
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.cloud.client.loadbalancer.LoadBalancedBackOffPolicyFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryListenerFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory;
import org.springframework.cloud.openfeign.FeignAutoConfiguration;
import org.springframework.cloud.openfeign.support.FeignHttpClientProperties;
import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
Expand Down Expand Up @@ -67,10 +65,8 @@ public CachingSpringLoadBalancerFactory cachingLBClientFactory(
@ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate")
public CachingSpringLoadBalancerFactory retryabeCachingLBClientFactory(
SpringClientFactory factory,
LoadBalancedRetryPolicyFactory retryPolicyFactory,
LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory,
LoadBalancedRetryListenerFactory loadBalancedRetryListenerFactory) {
return new CachingSpringLoadBalancerFactory(factory, retryPolicyFactory, loadBalancedBackOffPolicyFactory, loadBalancedRetryListenerFactory);
LoadBalancedRetryFactory retryFactory) {
return new CachingSpringLoadBalancerFactory(factory, retryFactory);
}

@Bean
Expand Down
52 changes: 9 additions & 43 deletions .../src/main/java/org/springframework/cloud/openfeign/ribbon/RetryableFeignLoadBalancer.java
View file
Expand Up @@ -24,12 +24,10 @@
import java.io.IOException;
import java.net.URI;
import org.springframework.cloud.client.ServiceInstance;
import org.springframework.cloud.client.loadbalancer.LoadBalancedBackOffPolicyFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRecoveryCallback;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryListenerFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory;
import org.springframework.cloud.client.loadbalancer.RibbonRecoveryCallback;
import org.springframework.cloud.client.loadbalancer.ServiceInstanceChooser;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancerClient;
import org.springframework.cloud.netflix.ribbon.RibbonProperties;
Expand All @@ -55,45 +53,13 @@
*/
public class RetryableFeignLoadBalancer extends FeignLoadBalancer implements ServiceInstanceChooser {

private final LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory;
private final LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory;
private final LoadBalancedRetryListenerFactory loadBalancedRetryListenerFactory;

@Deprecated
//TODO remove in 2.0.x
public RetryableFeignLoadBalancer(ILoadBalancer lb, IClientConfig clientConfig,
ServerIntrospector serverIntrospector, LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory) {
super(lb, clientConfig, serverIntrospector);
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.setRetryHandler(new DefaultLoadBalancerRetryHandler(clientConfig));
this.loadBalancedBackOffPolicyFactory = new LoadBalancedBackOffPolicyFactory.NoBackOffPolicyFactory();
this.loadBalancedRetryListenerFactory = new LoadBalancedRetryListenerFactory.DefaultRetryListenerFactory();
}

@Deprecated
//TODO remove in 2.0.x
public RetryableFeignLoadBalancer(ILoadBalancer lb, IClientConfig clientConfig,
ServerIntrospector serverIntrospector, LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory,
LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory) {
super(lb, clientConfig, serverIntrospector);
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.setRetryHandler(new DefaultLoadBalancerRetryHandler(clientConfig));
this.loadBalancedBackOffPolicyFactory = loadBalancedBackOffPolicyFactory == null ?
new LoadBalancedBackOffPolicyFactory.NoBackOffPolicyFactory() : loadBalancedBackOffPolicyFactory;
this.loadBalancedRetryListenerFactory = new LoadBalancedRetryListenerFactory.DefaultRetryListenerFactory();
}
private final LoadBalancedRetryFactory loadBalancedRetryFactory;

public RetryableFeignLoadBalancer(ILoadBalancer lb, IClientConfig clientConfig, ServerIntrospector serverIntrospector,
LoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory,
LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory,
LoadBalancedRetryListenerFactory loadBalancedRetryListenerFactory) {
LoadBalancedRetryFactory loadBalancedRetryFactory) {
super(lb, clientConfig, serverIntrospector);
this.loadBalancedRetryPolicyFactory = loadBalancedRetryPolicyFactory;
this.loadBalancedRetryFactory = loadBalancedRetryFactory;
this.setRetryHandler(new DefaultLoadBalancerRetryHandler(clientConfig));
this.loadBalancedBackOffPolicyFactory = loadBalancedBackOffPolicyFactory == null ?
new LoadBalancedBackOffPolicyFactory.NoBackOffPolicyFactory() : loadBalancedBackOffPolicyFactory;
this.loadBalancedRetryListenerFactory = loadBalancedRetryListenerFactory == null ?
new LoadBalancedRetryListenerFactory.DefaultRetryListenerFactory() : loadBalancedRetryListenerFactory;
}

@Override
Expand All @@ -109,11 +75,11 @@ public RibbonResponse execute(final RibbonRequest request, IClientConfig configO
else {
options = new Request.Options(this.connectTimeout, this.readTimeout);
}
final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory.create(this.getClientName(), this);
final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryFactory.createRetryPolicy(this.getClientName(), this);
RetryTemplate retryTemplate = new RetryTemplate();
BackOffPolicy backOffPolicy = loadBalancedBackOffPolicyFactory.createBackOffPolicy(this.getClientName());
BackOffPolicy backOffPolicy = loadBalancedRetryFactory.createBackOffPolicy(this.getClientName());
retryTemplate.setBackOffPolicy(backOffPolicy == null ? new NoBackOffPolicy() : backOffPolicy);
RetryListener[] retryListeners = this.loadBalancedRetryListenerFactory.createRetryListeners(this.getClientName());
RetryListener[] retryListeners = this.loadBalancedRetryFactory.createRetryListeners(this.getClientName());
if (retryListeners != null && retryListeners.length != 0) {
retryTemplate.setListeners(retryListeners);
}
Expand Down Expand Up @@ -143,7 +109,7 @@ public RibbonResponse doWithRetry(RetryContext retryContext) throws IOException
}
return new RibbonResponse(request.getUri(), response);
}
}, new RibbonRecoveryCallback<RibbonResponse, Response>() {
}, new LoadBalancedRecoveryCallback<RibbonResponse, Response>() {
@Override
protected RibbonResponse createResponse(Response response, URI uri) {
return new RibbonResponse(uri, response);
Expand Down
42 changes: 4 additions & 38 deletions ...ava/org/springframework/cloud/openfeign/ribbon/CachingSpringLoadBalancerFactoryTests.java
View file
Expand Up @@ -23,9 +23,7 @@
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.springframework.cloud.client.loadbalancer.LoadBalancedBackOffPolicyFactory;
import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryListenerFactory;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryPolicyFactory;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryFactory;
import org.springframework.cloud.netflix.ribbon.SpringClientFactory;

import static org.junit.Assert.assertNotNull;
Expand All @@ -42,13 +40,7 @@ public class CachingSpringLoadBalancerFactoryTests {
private SpringClientFactory delegate;

@Mock
private RibbonLoadBalancedRetryPolicyFactory loadBalancedRetryPolicyFactory;

@Mock
private LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory;

@Mock
private LoadBalancedRetryListenerFactory loadBalancedRetryListenerFactory;
private RibbonLoadBalancedRetryFactory loadBalancedRetryFactory;

private CachingSpringLoadBalancerFactory factory;

Expand All @@ -64,7 +56,7 @@ public void init() {
when(this.delegate.getClientConfig("client2")).thenReturn(config);

this.factory = new CachingSpringLoadBalancerFactory(this.delegate,
loadBalancedRetryPolicyFactory);
loadBalancedRetryFactory);
}

@Test
Expand Down Expand Up @@ -103,34 +95,8 @@ public void delegateCreatesWithRetry() {
config.set(CommonClientConfigKey.ConnectTimeout, 1000);
config.set(CommonClientConfigKey.ReadTimeout, 500);
when(this.delegate.getClientConfig("retry")).thenReturn(config);
CachingSpringLoadBalancerFactory factory = new CachingSpringLoadBalancerFactory(
this.delegate, loadBalancedRetryPolicyFactory, false);
CachingSpringLoadBalancerFactory factory = new CachingSpringLoadBalancerFactory(this.delegate, loadBalancedRetryFactory);
FeignLoadBalancer client = this.factory.create("retry");
assertNotNull("client was null", client);
}

@Test
public void delegateCreatesWithBackOff() {
IClientConfig config = new DefaultClientConfigImpl();
config.set(CommonClientConfigKey.ConnectTimeout, 1000);
config.set(CommonClientConfigKey.ReadTimeout, 500);
when(this.delegate.getClientConfig("retry")).thenReturn(config);
CachingSpringLoadBalancerFactory factory = new CachingSpringLoadBalancerFactory(
this.delegate, loadBalancedRetryPolicyFactory, loadBalancedBackOffPolicyFactory);
FeignLoadBalancer client = this.factory.create("retry");
assertNotNull("client was null", client);
}

@Test
public void delegateCreatesWithRetryListener() {
IClientConfig config = new DefaultClientConfigImpl();
config.set(CommonClientConfigKey.ConnectTimeout, 1000);
config.set(CommonClientConfigKey.ReadTimeout, 500);
when(this.delegate.getClientConfig("retry")).thenReturn(config);
CachingSpringLoadBalancerFactory factory = new CachingSpringLoadBalancerFactory(
this.delegate, loadBalancedRetryPolicyFactory, loadBalancedBackOffPolicyFactory, loadBalancedRetryListenerFactory);
FeignLoadBalancer client = this.factory.create("retry");
assertNotNull("client was null", client);
}

}
5 changes: 1 addition & 4 deletions ...core/src/test/java/org/springframework/cloud/openfeign/ribbon/FeignRibbonClientTests.java
View file
Expand Up @@ -32,7 +32,6 @@
import org.junit.Before;
import org.junit.Test;
import org.springframework.cloud.netflix.ribbon.DefaultServerIntrospector;
import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryPolicyFactory;
import org.springframework.cloud.netflix.ribbon.ServerIntrospector;
import org.springframework.cloud.netflix.ribbon.SpringClientFactory;

Expand All @@ -48,7 +47,6 @@ public class FeignRibbonClientTests {

private AbstractLoadBalancer loadBalancer = mock(AbstractLoadBalancer.class);
private Client delegate = mock(Client.class);
private RibbonLoadBalancedRetryPolicyFactory retryPolicyFactory = mock(RibbonLoadBalancedRetryPolicyFactory.class);

private SpringClientFactory factory = new SpringClientFactory() {
@Override
Expand Down Expand Up @@ -77,8 +75,7 @@ public ILoadBalancer getLoadBalancer(String name) {

// Even though we don't maintain FeignRibbonClient, keep these tests
// around to make sure the expected behaviour doesn't break
private Client client = new LoadBalancerFeignClient(this.delegate, new CachingSpringLoadBalancerFactory(this.factory,
retryPolicyFactory), this.factory);
private Client client = new LoadBalancerFeignClient(this.delegate, new CachingSpringLoadBalancerFactory(this.factory), this.factory);

@Before
public void init() {
Expand Down

0 comments on commit 485583d

Please sign in to comment.