Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Spring Retry Code #10

Merged
merged 2 commits into from Mar 13, 2018
Merged

Refactor Spring Retry Code #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8f9df0
Refactor spring retry code
ryanjbaxter Mar 10, 2018
fab6a9b
Merge remote-tracking branch 'Upstream/master' into spring-retry-refa…
ryanjbaxter Mar 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 6 additions & 53 deletions ...ain/java/org/springframework/cloud/openfeign/ribbon/CachingSpringLoadBalancerFactory.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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