diff --git a/docs/src/main/asciidoc/_configprops.adoc b/docs/src/main/asciidoc/_configprops.adoc index b046388d1..e980076cf 100644 --- a/docs/src/main/asciidoc/_configprops.adoc +++ b/docs/src/main/asciidoc/_configprops.adoc @@ -35,6 +35,7 @@ |spring.cloud.loadbalancer.health-check.interval | 25s | Interval for rerunning the HealthCheck scheduler. |spring.cloud.loadbalancer.health-check.path | | |spring.cloud.loadbalancer.hint | | Allows setting the value of hint that is passed on to the LoadBalancer request and can subsequently be used in {@link ReactiveLoadBalancer} implementations. +|spring.cloud.loadbalancer.retry.avoid-previous-instance | true | Enables wrapping ServiceInstanceListSupplier beans with `RetryAwareServiceInstanceListSupplier` if Spring-Retry is in the classpath. |spring.cloud.loadbalancer.retry.enabled | true | |spring.cloud.loadbalancer.retry.max-retries-on-next-service-instance | 1 | Number of retries to be executed on the next ServiceInstance. A ServiceInstance is chosen before each retry call. |spring.cloud.loadbalancer.retry.max-retries-on-same-service-instance | 0 | Number of retries to be executed on the same ServiceInstance. diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 2ae870414..d26d4b9ad 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -448,6 +448,9 @@ You can set: - `spring.cloud.loadbalancer.retry.maxRetriesOnSameServiceInstance` - indicates how many times a request should be retried on the same `ServiceInstance` (counted separately for every selected instance) - `spring.cloud.loadbalancer.retry.maxRetriesOnNextServiceInstance` - indicates how many times a request should be retried a newly selected `ServiceInstance` - `spring.cloud.loadbalancer.retry.retryableStatusCodes` - the status codes on which to always retry a failed request. + +NOTE: For load-balanced retries, by default, we wrap the `ServiceInstanceListSupplier` bean with `RetryAwareServiceInstanceListSupplier` to select a different instance from the one previously chosen, if available. You can disable this behavior by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance` to `false`. + ==== [source,java,indent=0] ---- diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicy.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicy.java index 1ef2f0ede..ebf8c428c 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicy.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicy.java @@ -25,6 +25,7 @@ * requests. * * @author Ryan Baxter + * @author Olga Maciaszek-Sharma */ public class InterceptorRetryPolicy implements RetryPolicy { @@ -56,7 +57,7 @@ public boolean canRetry(RetryContext context) { LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; if (lbContext.getRetryCount() == 0 && lbContext.getServiceInstance() == null) { // We haven't even tried to make the request yet so return true so we do - lbContext.setServiceInstance(serviceInstanceChooser.choose(serviceName)); + lbContext.setServiceInstance(null); return true; } return policy.canRetryNextServer(lbContext); diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancedRetryContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancedRetryContext.java index db83052cd..9789e0465 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancedRetryContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancedRetryContext.java @@ -25,6 +25,7 @@ * {@link RetryContext} for load-balanced retries. * * @author Ryan Baxter + * @author Olga Maciaszek-Sharma */ public class LoadBalancedRetryContext extends RetryContextSupport { @@ -32,6 +33,8 @@ public class LoadBalancedRetryContext extends RetryContextSupport { private ServiceInstance serviceInstance; + private ServiceInstance previousServiceInstance; + /** * Creates a new load-balanced context. * @param parent The parent context. @@ -71,7 +74,16 @@ public ServiceInstance getServiceInstance() { * @param serviceInstance The service instance to use during the retry. */ public void setServiceInstance(ServiceInstance serviceInstance) { + setPreviousServiceInstance(this.serviceInstance); this.serviceInstance = serviceInstance; } + public ServiceInstance getPreviousServiceInstance() { + return previousServiceInstance; + } + + public void setPreviousServiceInstance(ServiceInstance previousServiceInstance) { + this.previousServiceInstance = previousServiceInstance; + } + } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java index 737c0e91f..96fd9d232 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java @@ -44,11 +44,12 @@ * @author Dave Syer * @author Will Tran * @author Gang Li + * @author Olga Maciaszek-Sharma */ @Configuration(proxyBeanMethods = false) @ConditionalOnClass(RestTemplate.class) @ConditionalOnBean(LoadBalancerClient.class) -@EnableConfigurationProperties(LoadBalancerRetryProperties.class) +@EnableConfigurationProperties(LoadBalancerProperties.class) public class LoadBalancerAutoConfiguration { @LoadBalanced @@ -125,11 +126,11 @@ public static class RetryInterceptorAutoConfiguration { @Bean @ConditionalOnMissingBean public RetryLoadBalancerInterceptor loadBalancerInterceptor(LoadBalancerClient loadBalancerClient, - LoadBalancerRetryProperties retryProperties, LoadBalancerRequestFactory requestFactory, - LoadBalancedRetryFactory loadBalancedRetryFactory, LoadBalancerProperties properties, + LoadBalancerProperties properties, LoadBalancerRequestFactory requestFactory, + LoadBalancedRetryFactory loadBalancedRetryFactory, ReactiveLoadBalancer.Factory loadBalancerFactory) { - return new RetryLoadBalancerInterceptor(loadBalancerClient, retryProperties, requestFactory, - loadBalancedRetryFactory, properties, loadBalancerFactory); + return new RetryLoadBalancerInterceptor(loadBalancerClient, properties, requestFactory, + loadBalancedRetryFactory, loadBalancerFactory); } @Bean diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRetryProperties.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRetryProperties.java deleted file mode 100644 index fbded2dd7..000000000 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRetryProperties.java +++ /dev/null @@ -1,105 +0,0 @@ -/* - * Copyright 2012-2020 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.cloud.client.loadbalancer; - -import java.util.HashSet; -import java.util.Set; - -import org.springframework.boot.context.properties.ConfigurationProperties; -import org.springframework.http.HttpMethod; - -/** - * Configuration properties for the {@link LoadBalancerClient}. - * - * @author Ryan Baxter - */ -@ConfigurationProperties("spring.cloud.loadbalancer.retry") -public class LoadBalancerRetryProperties { - - private boolean enabled = true; - - /** - * Indicates retries should be attempted on operations other than - * {@link HttpMethod#GET}. - */ - private boolean retryOnAllOperations = false; - - /** - * Number of retries to be executed on the same ServiceInstance. - */ - private int maxRetriesOnSameServiceInstance = 0; - - /** - * Number of retries to be executed on the next ServiceInstance. A - * ServiceInstance is chosen before each retry call. - */ - private int maxRetriesOnNextServiceInstance = 1; - - /** - * A {@link Set} of status codes that should trigger a retry. - */ - private Set retryableStatusCodes = new HashSet<>(); - - /** - * Returns true if the load balancer should retry failed requests. - * @return True if the load balancer should retry failed requests; false otherwise. - */ - public boolean isEnabled() { - return this.enabled; - } - - /** - * Sets whether the load balancer should retry failed requests. - * @param enabled Whether the load balancer should retry failed requests. - */ - public void setEnabled(boolean enabled) { - this.enabled = enabled; - } - - public boolean isRetryOnAllOperations() { - return retryOnAllOperations; - } - - public void setRetryOnAllOperations(boolean retryOnAllOperations) { - this.retryOnAllOperations = retryOnAllOperations; - } - - public int getMaxRetriesOnSameServiceInstance() { - return maxRetriesOnSameServiceInstance; - } - - public void setMaxRetriesOnSameServiceInstance(int maxRetriesOnSameServiceInstance) { - this.maxRetriesOnSameServiceInstance = maxRetriesOnSameServiceInstance; - } - - public int getMaxRetriesOnNextServiceInstance() { - return maxRetriesOnNextServiceInstance; - } - - public void setMaxRetriesOnNextServiceInstance(int maxRetriesOnNextServiceInstance) { - this.maxRetriesOnNextServiceInstance = maxRetriesOnNextServiceInstance; - } - - public Set getRetryableStatusCodes() { - return retryableStatusCodes; - } - - public void setRetryableStatusCodes(Set retryableStatusCodes) { - this.retryableStatusCodes = retryableStatusCodes; - } - -} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java index 255e8721f..dc79c1dce 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java @@ -52,18 +52,15 @@ public class RetryLoadBalancerInterceptor implements ClientHttpRequestIntercepto private final LoadBalancedRetryFactory lbRetryFactory; - private final LoadBalancerRetryProperties retryProperties; - private final ReactiveLoadBalancer.Factory loadBalancerFactory; - public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer, LoadBalancerRetryProperties retryProperties, + public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer, LoadBalancerProperties properties, LoadBalancerRequestFactory requestFactory, LoadBalancedRetryFactory lbRetryFactory, - LoadBalancerProperties properties, ReactiveLoadBalancer.Factory loadBalancerFactory) { + ReactiveLoadBalancer.Factory loadBalancerFactory) { this.loadBalancer = loadBalancer; - this.retryProperties = retryProperties; + this.properties = properties; this.requestFactory = requestFactory; this.lbRetryFactory = lbRetryFactory; - this.properties = properties; this.loadBalancerFactory = loadBalancerFactory; } @@ -85,11 +82,21 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body .getSupportedLifecycleProcessors( loadBalancerFactory.getInstances(serviceName, LoadBalancerLifecycle.class), HttpRequestContext.class, ClientHttpResponse.class, ServiceInstance.class); - String hint = getHint(serviceName); - DefaultRequest lbRequest = new DefaultRequest<>(new HttpRequestContext(request, hint)); - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); if (serviceInstance == null) { + ServiceInstance previousServiceInstance = null; + if (context instanceof LoadBalancedRetryContext) { + LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; + previousServiceInstance = lbContext.getPreviousServiceInstance(); + } + String hint = getHint(serviceName); + DefaultRequest lbRequest = new DefaultRequest<>( + new RetryableRequestContext(previousServiceInstance, request, hint)); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); serviceInstance = loadBalancer.choose(serviceName, lbRequest); + if (context instanceof LoadBalancedRetryContext) { + LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; + lbContext.setServiceInstance(serviceInstance); + } } Response lbResponse = new DefaultResponse(serviceInstance); if (serviceInstance == null) { @@ -127,7 +134,7 @@ private RetryTemplate createRetryTemplate(String serviceName, HttpRequest reques if (retryListeners != null && retryListeners.length != 0) { template.setListeners(retryListeners); } - template.setRetryPolicy(!retryProperties.isEnabled() || retryPolicy == null ? new NeverRetryPolicy() + template.setRetryPolicy(!properties.getRetry().isEnabled() || retryPolicy == null ? new NeverRetryPolicy() : new InterceptorRetryPolicy(request, retryPolicy, loadBalancer, serviceName)); return template; } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java new file mode 100644 index 000000000..20595fa29 --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java @@ -0,0 +1,49 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.client.loadbalancer; + +import org.springframework.cloud.client.ServiceInstance; + +/** + * A request context object that allows storing information on previously used service + * instances. + * + * @author Olga Maciaszek-Sharma + */ +public class RetryableRequestContext extends DefaultRequestContext { + + private final ServiceInstance previousServiceInstance; + + public RetryableRequestContext(ServiceInstance previousServiceInstance) { + this.previousServiceInstance = previousServiceInstance; + } + + public RetryableRequestContext(ServiceInstance previousServiceInstance, Object clientRequest) { + super(clientRequest); + this.previousServiceInstance = previousServiceInstance; + } + + public RetryableRequestContext(ServiceInstance previousServiceInstance, Object clientRequest, String hint) { + super(clientRequest, hint); + this.previousServiceInstance = previousServiceInstance; + } + + public ServiceInstance getPreviousServiceInstance() { + return previousServiceInstance; + } + +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerProperties.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerProperties.java index 92a46fbf1..5bbc89df1 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerProperties.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerProperties.java @@ -17,9 +17,12 @@ package org.springframework.cloud.client.loadbalancer.reactive; import java.time.Duration; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.http.HttpMethod; import org.springframework.util.LinkedCaseInsensitiveMap; /** @@ -43,6 +46,11 @@ public class LoadBalancerProperties { */ private Map hint = new LinkedCaseInsensitiveMap<>(); + /** + * Properties for Spring-Retry support in Spring Cloud LoadBalancer. + */ + private Retry retry = new Retry(); + public HealthCheck getHealthCheck() { return healthCheck; } @@ -59,6 +67,14 @@ public void setHint(Map hint) { this.hint = hint; } + public Retry getRetry() { + return retry; + } + + public void setRetry(Retry retry) { + this.retry = retry; + } + public static class HealthCheck { /** @@ -99,4 +115,81 @@ public void setInterval(Duration interval) { } + public static class Retry { + + private boolean enabled = true; + + /** + * Indicates retries should be attempted on operations other than + * {@link HttpMethod#GET}. + */ + private boolean retryOnAllOperations = false; + + /** + * Number of retries to be executed on the same ServiceInstance. + */ + private int maxRetriesOnSameServiceInstance = 0; + + /** + * Number of retries to be executed on the next ServiceInstance. A + * ServiceInstance is chosen before each retry call. + */ + private int maxRetriesOnNextServiceInstance = 1; + + /** + * A {@link Set} of status codes that should trigger a retry. + */ + private Set retryableStatusCodes = new HashSet<>(); + + /** + * Returns true if the load balancer should retry failed requests. + * @return True if the load balancer should retry failed requests; false + * otherwise. + */ + public boolean isEnabled() { + return this.enabled; + } + + /** + * Sets whether the load balancer should retry failed requests. + * @param enabled Whether the load balancer should retry failed requests. + */ + public void setEnabled(boolean enabled) { + this.enabled = enabled; + } + + public boolean isRetryOnAllOperations() { + return retryOnAllOperations; + } + + public void setRetryOnAllOperations(boolean retryOnAllOperations) { + this.retryOnAllOperations = retryOnAllOperations; + } + + public int getMaxRetriesOnSameServiceInstance() { + return maxRetriesOnSameServiceInstance; + } + + public void setMaxRetriesOnSameServiceInstance(int maxRetriesOnSameServiceInstance) { + this.maxRetriesOnSameServiceInstance = maxRetriesOnSameServiceInstance; + } + + public int getMaxRetriesOnNextServiceInstance() { + return maxRetriesOnNextServiceInstance; + } + + public void setMaxRetriesOnNextServiceInstance(int maxRetriesOnNextServiceInstance) { + this.maxRetriesOnNextServiceInstance = maxRetriesOnNextServiceInstance; + } + + public Set getRetryableStatusCodes() { + return retryableStatusCodes; + } + + public void setRetryableStatusCodes(Set retryableStatusCodes) { + this.retryableStatusCodes = retryableStatusCodes; + } + + } + } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/AbstractLoadBalancerAutoConfigurationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/AbstractLoadBalancerAutoConfigurationTests.java index 81a95dfed..0c6d438bc 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/AbstractLoadBalancerAutoConfigurationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/AbstractLoadBalancerAutoConfigurationTests.java @@ -28,7 +28,6 @@ import org.springframework.boot.builder.SpringApplicationBuilder; import org.springframework.cloud.client.DefaultServiceInstance; import org.springframework.cloud.client.ServiceInstance; -import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.client.loadbalancer.reactive.ReactiveLoadBalancer; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Bean; @@ -100,11 +99,6 @@ LoadBalancerClient loadBalancerClient() { return new NoopLoadBalancerClient(); } - @Bean - LoadBalancerProperties loadBalancerProperties() { - return new LoadBalancerProperties(); - } - @Bean ReactiveLoadBalancer.Factory loadBalancerFactory() { return new TestLoadBalancerFactory(); diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicyTest.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicyTest.java index b7af67ec3..3301b5256 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicyTest.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicyTest.java @@ -22,7 +22,6 @@ import org.junit.runner.RunWith; import org.mockito.junit.MockitoJUnitRunner; -import org.springframework.cloud.client.ServiceInstance; import org.springframework.http.HttpRequest; import org.springframework.retry.RetryContext; @@ -35,6 +34,7 @@ /** * @author Ryan Baxter + * @author Olga Maciaszek-Sharma */ @RunWith(MockitoJUnitRunner.class) public class InterceptorRetryPolicyTest { @@ -69,10 +69,8 @@ public void canRetryBeforeExecution() { serviceInstanceChooser, serviceName); LoadBalancedRetryContext context = mock(LoadBalancedRetryContext.class); when(context.getRetryCount()).thenReturn(0); - ServiceInstance serviceInstance = mock(ServiceInstance.class); - when(serviceInstanceChooser.choose(eq(serviceName))).thenReturn(serviceInstance); then(interceptorRetryPolicy.canRetry(context)).isTrue(); - verify(context, times(1)).setServiceInstance(eq(serviceInstance)); + verify(context, times(1)).setServiceInstance(eq(null)); } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestFactoryConfigurationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestFactoryConfigurationTests.java index bd439c10f..f072c25f9 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestFactoryConfigurationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestFactoryConfigurationTests.java @@ -26,7 +26,6 @@ import org.springframework.boot.WebApplicationType; import org.springframework.boot.builder.SpringApplicationBuilder; import org.springframework.cloud.client.ServiceInstance; -import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.client.loadbalancer.reactive.ReactiveLoadBalancer; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Bean; @@ -163,11 +162,6 @@ public LoadBalancerClient loadBalancerClient() { return mock(LoadBalancerClient.class); } - @Bean - LoadBalancerProperties loadBalancerProperties() { - return new LoadBalancerProperties(); - } - @SuppressWarnings("unchecked") @Bean ReactiveLoadBalancer.Factory factory() { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java index 931e6307c..df3ebf68a 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java @@ -74,8 +74,6 @@ public class RetryLoadBalancerInterceptorTests { private LoadBalancerClient client; - private LoadBalancerRetryProperties retryProperties; - private LoadBalancerRequestFactory lbRequestFactory; private final LoadBalancedRetryFactory loadBalancedRetryFactory = new LoadBalancedRetryFactory() { @@ -88,7 +86,6 @@ public class RetryLoadBalancerInterceptorTests { @Before public void setUp() { client = mock(LoadBalancerClient.class); - retryProperties = new LoadBalancerRetryProperties(); lbRequestFactory = mock(LoadBalancerRequestFactory.class); properties = new LoadBalancerProperties(); lbFactory = mock(ReactiveLoadBalancer.Factory.class); @@ -97,7 +94,6 @@ public void setUp() { @After public void tearDown() { client = null; - retryProperties = null; } @Test(expected = IOException.class) @@ -108,9 +104,9 @@ public void interceptDisableRetry() throws Throwable { when(client.choose(eq("foo"), any())).thenReturn(serviceInstance); when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenThrow(new IOException()); - retryProperties.setEnabled(false); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, loadBalancedRetryFactory, properties, lbFactory); + properties.getRetry().setEnabled(false); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, loadBalancedRetryFactory, lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); @@ -124,9 +120,9 @@ public void interceptDisableRetry() throws Throwable { public void interceptInvalidHost() throws Throwable { HttpRequest request = mock(HttpRequest.class); when(request.getURI()).thenReturn(new URI("http://foo_underscore")); - retryProperties.setEnabled(true); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, loadBalancedRetryFactory, properties, lbFactory); + properties.getRetry().setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, loadBalancedRetryFactory, lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); interceptor.intercept(request, body, execution); @@ -142,9 +138,9 @@ public void interceptNeverRetry() throws Throwable { when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenReturn(clientHttpResponse); when(lbRequestFactory.createRequest(any(), any(), any())).thenReturn(mock(LoadBalancerRequest.class)); - retryProperties.setEnabled(true); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, loadBalancedRetryFactory, properties, lbFactory); + properties.getRetry().setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, loadBalancedRetryFactory, lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); interceptor.intercept(request, body, execution); @@ -162,9 +158,9 @@ public void interceptSuccess() throws Throwable { when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenReturn(clientHttpResponse); when(lbRequestFactory.createRequest(any(), any(), any())).thenReturn(mock(LoadBalancerRequest.class)); - retryProperties.setEnabled(true); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, new MyLoadBalancedRetryFactory(policy), properties, lbFactory); + properties.getRetry().setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, new MyLoadBalancedRetryFactory(policy), lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); ClientHttpResponse rsp = interceptor.intercept(request, body, execution); @@ -188,9 +184,9 @@ public void interceptRetryOnStatusCode() throws Throwable { when(client.choose(eq("foo"), any())).thenReturn(serviceInstance); when(client.execute(eq("foo"), eq(serviceInstance), nullable(LoadBalancerRequest.class))) .thenReturn(clientHttpResponseNotFound).thenReturn(clientHttpResponseOk); - retryProperties.setEnabled(true); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, new MyLoadBalancedRetryFactory(policy), properties, lbFactory); + properties.getRetry().setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, new MyLoadBalancedRetryFactory(policy), lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); ClientHttpResponse rsp = interceptor.intercept(request, body, execution); @@ -219,11 +215,11 @@ public void interceptRetryFailOnStatusCode() throws Throwable { ArgumentMatchers.>any())) .thenReturn(clientHttpResponseNotFound); - retryProperties.setEnabled(true); + properties.getRetry().setEnabled(true); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, new MyLoadBalancedRetryFactory(policy), properties, lbFactory); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, new MyLoadBalancedRetryFactory(policy), lbFactory); ClientHttpResponse rsp = interceptor.intercept(request, body, execution); verify(client, times(1)).execute(eq("foo"), eq(serviceInstance), @@ -251,9 +247,9 @@ public void interceptRetry() throws Throwable { when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenThrow(new IOException()).thenReturn(clientHttpResponse); when(lbRequestFactory.createRequest(any(), any(), any())).thenReturn(mock(LoadBalancerRequest.class)); - retryProperties.setEnabled(true); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, new MyLoadBalancedRetryFactory(policy, backOffPolicy), properties, lbFactory); + properties.getRetry().setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, new MyLoadBalancedRetryFactory(policy, backOffPolicy), lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); ClientHttpResponse rsp = interceptor.intercept(request, body, execution); @@ -275,9 +271,9 @@ public void interceptFailedRetry() throws Exception { when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenThrow(new IOException()).thenReturn(clientHttpResponse); when(lbRequestFactory.createRequest(any(), any(), any())).thenReturn(mock(LoadBalancerRequest.class)); - retryProperties.setEnabled(true); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, new MyLoadBalancedRetryFactory(policy), properties, lbFactory); + properties.getRetry().setEnabled(true); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, new MyLoadBalancedRetryFactory(policy), lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); interceptor.intercept(request, body, execution); @@ -300,13 +296,13 @@ public void retryListenerTest() throws Throwable { when(client.choose(eq("listener"), any())).thenReturn(serviceInstance); when(client.execute(eq("listener"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenThrow(new IOException()).thenReturn(clientHttpResponse); - retryProperties.setEnabled(true); + properties.getRetry().setEnabled(true); MyRetryListener retryListener = new MyRetryListener(); when(lbRequestFactory.createRequest(any(), any(), any())).thenReturn(mock(LoadBalancerRequest.class)); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, lbRequestFactory, new MyLoadBalancedRetryFactory(policy, backOffPolicy, new RetryListener[] { retryListener }), - properties, lbFactory); + lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); ClientHttpResponse rsp = interceptor.intercept(request, body, execution); @@ -329,10 +325,10 @@ public void retryWithDefaultConstructorTest() throws Throwable { when(client.choose(eq("default"), any())).thenReturn(serviceInstance); when(client.execute(eq("default"), eq(serviceInstance), any(LoadBalancerRequest.class))) .thenThrow(new IOException()).thenReturn(clientHttpResponse); - retryProperties.setEnabled(true); + properties.getRetry().setEnabled(true); when(lbRequestFactory.createRequest(any(), any(), any())).thenReturn(mock(LoadBalancerRequest.class)); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, new MyLoadBalancedRetryFactory(policy, backOffPolicy), properties, lbFactory); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, new MyLoadBalancedRetryFactory(policy, backOffPolicy), lbFactory); byte[] body = new byte[] {}; ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); ClientHttpResponse rsp = interceptor.intercept(request, body, execution); @@ -348,17 +344,17 @@ public void retryListenerTestNoRetry() throws Throwable { when(request.getURI()).thenReturn(new URI("http://noRetry")); LoadBalancedRetryPolicy policy = mock(LoadBalancedRetryPolicy.class); MyBackOffPolicy backOffPolicy = new MyBackOffPolicy(); - retryProperties.setEnabled(true); + properties.getRetry().setEnabled(true); RetryListener myRetryListener = new RetryListenerSupport() { @Override public boolean open(RetryContext context, RetryCallback callback) { return false; } }; - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, lbRequestFactory, new MyLoadBalancedRetryFactory(policy, backOffPolicy, new RetryListener[] { myRetryListener }), - properties, lbFactory); + lbFactory); ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class); interceptor.intercept(request, new byte[] {}, execution); } @@ -372,8 +368,8 @@ public void shouldNotDuplicateLifecycleCalls() throws IOException, URISyntaxExce HttpRequest request = mock(HttpRequest.class); when(request.getURI()).thenReturn(new URI("http://test")); TestLoadBalancerClient client = new TestLoadBalancerClient(); - RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryProperties, - lbRequestFactory, loadBalancedRetryFactory, properties, lbFactory); + RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, properties, + lbRequestFactory, loadBalancedRetryFactory, lbFactory); interceptor.intercept(request, new byte[] {}, mock(ClientHttpRequestExecution.class)); diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java index 124bbff04..747b5fecf 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java @@ -16,7 +16,10 @@ package org.springframework.cloud.loadbalancer.annotation; +import org.springframework.boot.autoconfigure.AutoConfigureAfter; +import org.springframework.boot.autoconfigure.condition.AllNestedConditions; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.cloud.client.ConditionalOnBlockingDiscoveryEnabled; @@ -26,14 +29,18 @@ import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; import org.springframework.cloud.loadbalancer.core.ReactorLoadBalancer; +import org.springframework.cloud.loadbalancer.core.RetryAwareServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.RoundRobinLoadBalancer; import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Primary; import org.springframework.core.annotation.Order; import org.springframework.core.env.Environment; +import org.springframework.retry.support.RetryTemplate; /** * @author Spencer Gibb @@ -129,4 +136,42 @@ public ServiceInstanceListSupplier healthCheckDiscoveryClientServiceInstanceList } + @Configuration(proxyBeanMethods = false) + @ConditionalOnBlockingDiscoveryEnabled + @ConditionalOnClass(RetryTemplate.class) + @Conditional(OnAvoidPreviousInstanceAndRetryEnabledCondition.class) + @AutoConfigureAfter(BlockingSupportConfiguration.class) + @ConditionalOnBean(ServiceInstanceListSupplier.class) + public static class BlockingRetryConfiguration { + + @Bean + @ConditionalOnBean(DiscoveryClient.class) + @Primary + public ServiceInstanceListSupplier retryAwareDiscoveryClientServiceInstanceListSupplier( + ServiceInstanceListSupplier delegate) { + return new RetryAwareServiceInstanceListSupplier(delegate); + } + + } + + static final class OnAvoidPreviousInstanceAndRetryEnabledCondition extends AllNestedConditions { + + private OnAvoidPreviousInstanceAndRetryEnabledCondition() { + super(ConfigurationPhase.REGISTER_BEAN); + } + + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true", + matchIfMissing = true) + static class LoadBalancerRetryEnabled { + + } + + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoid-previous-instance", havingValue = "true", + matchIfMissing = true) + static class AvoidPreviousInstanceEnabled { + + } + + } + } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryFactory.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryFactory.java index ecf995202..630678a82 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryFactory.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryFactory.java @@ -18,8 +18,8 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy; -import org.springframework.cloud.client.loadbalancer.LoadBalancerRetryProperties; import org.springframework.cloud.client.loadbalancer.ServiceInstanceChooser; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; /** @@ -31,15 +31,15 @@ */ public class BlockingLoadBalancedRetryFactory implements LoadBalancedRetryFactory { - private final LoadBalancerRetryProperties retryProperties; + private final LoadBalancerProperties loadBalancerProperties; - public BlockingLoadBalancedRetryFactory(LoadBalancerRetryProperties retryProperties) { - this.retryProperties = retryProperties; + public BlockingLoadBalancedRetryFactory(LoadBalancerProperties loadBalancerProperties) { + this.loadBalancerProperties = loadBalancerProperties; } @Override public LoadBalancedRetryPolicy createRetryPolicy(String serviceId, ServiceInstanceChooser serviceInstanceChooser) { - return new BlockingLoadBalancedRetryPolicy(serviceId, serviceInstanceChooser, retryProperties); + return new BlockingLoadBalancedRetryPolicy(loadBalancerProperties); } } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicy.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicy.java index c03607df7..930b63917 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicy.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicy.java @@ -18,8 +18,7 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy; -import org.springframework.cloud.client.loadbalancer.LoadBalancerRetryProperties; -import org.springframework.cloud.client.loadbalancer.ServiceInstanceChooser; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; import org.springframework.http.HttpMethod; @@ -33,37 +32,30 @@ */ public class BlockingLoadBalancedRetryPolicy implements LoadBalancedRetryPolicy { - private final LoadBalancerRetryProperties retryProperties; - - private final ServiceInstanceChooser serviceInstanceChooser; - - private final String serviceId; + private final LoadBalancerProperties properties; private int sameServerCount = 0; private int nextServerCount = 0; - public BlockingLoadBalancedRetryPolicy(String serviceId, ServiceInstanceChooser serviceInstanceChooser, - LoadBalancerRetryProperties retryProperties) { - this.serviceId = serviceId; - this.serviceInstanceChooser = serviceInstanceChooser; - this.retryProperties = retryProperties; + public BlockingLoadBalancedRetryPolicy(LoadBalancerProperties properties) { + this.properties = properties; } public boolean canRetry(LoadBalancedRetryContext context) { HttpMethod method = context.getRequest().getMethod(); - return HttpMethod.GET.equals(method) || retryProperties.isRetryOnAllOperations(); + return HttpMethod.GET.equals(method) || properties.getRetry().isRetryOnAllOperations(); } @Override public boolean canRetrySameServer(LoadBalancedRetryContext context) { - return sameServerCount < retryProperties.getMaxRetriesOnSameServiceInstance() && canRetry(context); + return sameServerCount < properties.getRetry().getMaxRetriesOnSameServiceInstance() && canRetry(context); } @Override public boolean canRetryNextServer(LoadBalancedRetryContext context) { // After the failure, we increment first and then check, hence the equality check - return nextServerCount <= retryProperties.getMaxRetriesOnNextServiceInstance() && canRetry(context); + return nextServerCount <= properties.getRetry().getMaxRetriesOnNextServiceInstance() && canRetry(context); } @Override @@ -81,7 +73,10 @@ public void registerThrowable(LoadBalancedRetryContext context, Throwable throwa context.setExhaustedOnly(); } else { - context.setServiceInstance(serviceInstanceChooser.choose(serviceId)); + // We want the service instance to be set by + // `RetryLoadBalancerInterceptor` + // in order to get the entire data of the request + context.setServiceInstance(null); } } else { @@ -91,7 +86,7 @@ public void registerThrowable(LoadBalancedRetryContext context, Throwable throwa @Override public boolean retryableStatusCode(int statusCode) { - return retryProperties.getRetryableStatusCodes().contains(statusCode); + return properties.getRetry().getRetryableStatusCodes().contains(statusCode); } } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/BlockingLoadBalancerClientAutoConfiguration.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/BlockingLoadBalancerClientAutoConfiguration.java index 1899cae04..62b030beb 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/BlockingLoadBalancerClientAutoConfiguration.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/BlockingLoadBalancerClientAutoConfiguration.java @@ -25,7 +25,6 @@ import org.springframework.cloud.client.loadbalancer.AsyncLoadBalancerAutoConfiguration; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; -import org.springframework.cloud.client.loadbalancer.LoadBalancerRetryProperties; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.loadbalancer.annotation.LoadBalancerClients; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; @@ -60,13 +59,13 @@ public LoadBalancerClient blockingLoadBalancerClient(LoadBalancerClientFactory l @Configuration @ConditionalOnClass(RetryTemplate.class) - @EnableConfigurationProperties(LoadBalancerRetryProperties.class) + @EnableConfigurationProperties(LoadBalancerProperties.class) protected static class BlockingLoadBalancerRetryConfig { @Bean @ConditionalOnMissingBean - LoadBalancedRetryFactory loadBalancedRetryFactory(LoadBalancerRetryProperties retryProperties) { - return new BlockingLoadBalancedRetryFactory(retryProperties); + LoadBalancedRetryFactory loadBalancedRetryFactory(LoadBalancerProperties properties) { + return new BlockingLoadBalancedRetryFactory(properties); } } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplier.java new file mode 100644 index 000000000..98691671b --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplier.java @@ -0,0 +1,85 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.loadbalancer.core; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import reactor.core.publisher.Flux; + +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; + +/** + * A {@link ServiceInstanceListSupplier} implementation that avoids picking the same + * service instance while retrying requests. + * + * @author Olga Maciaszek-Sharma + * @since 3.0.0 + */ +public class RetryAwareServiceInstanceListSupplier extends DelegatingServiceInstanceListSupplier { + + private final Log LOG = LogFactory.getLog(RetryAwareServiceInstanceListSupplier.class); + + public RetryAwareServiceInstanceListSupplier(ServiceInstanceListSupplier delegate) { + super(delegate); + } + + @Override + public String getServiceId() { + return delegate.getServiceId(); + } + + @Override + public Flux> get(Request request) { + if (!(request.getContext() instanceof RetryableRequestContext)) { + return get(); + } + RetryableRequestContext context = (RetryableRequestContext) request.getContext(); + ServiceInstance previousServiceInstance = context.getPreviousServiceInstance(); + if (previousServiceInstance == null) { + return get(); + } + return get().map(instances -> filteredByPreviousInstance(instances, previousServiceInstance)); + } + + private List filteredByPreviousInstance(List instances, + ServiceInstance previousServiceInstance) { + List filteredInstances = new ArrayList<>(instances); + if (previousServiceInstance != null) { + filteredInstances.remove(previousServiceInstance); + } + if (filteredInstances.size() > 0) { + return filteredInstances; + } + if (LOG.isWarnEnabled()) { + LOG.warn(String.format( + "No instances found after removing previously used service instance from the search (%s). Returning all found instances.", + previousServiceInstance)); + } + return instances; + } + + @Override + public Flux> get() { + return delegate.get(); + } + +} diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RoundRobinLoadBalancer.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RoundRobinLoadBalancer.java index 0d9cd0ae9..f7a53534c 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RoundRobinLoadBalancer.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RoundRobinLoadBalancer.java @@ -41,11 +41,11 @@ public class RoundRobinLoadBalancer implements ReactorServiceInstanceLoadBalance private static final Log log = LogFactory.getLog(RoundRobinLoadBalancer.class); - private final AtomicInteger position; + final AtomicInteger position; - private ObjectProvider serviceInstanceListSupplierProvider; + final String serviceId; - private final String serviceId; + ObjectProvider serviceInstanceListSupplierProvider; /** * @param serviceInstanceListSupplierProvider a provider of @@ -78,12 +78,14 @@ public RoundRobinLoadBalancer(ObjectProvider servic public Mono> choose(Request request) { ServiceInstanceListSupplier supplier = serviceInstanceListSupplierProvider .getIfAvailable(NoopServiceInstanceListSupplier::new); - return supplier.get().next().map(this::getInstanceResponse); + return supplier.get(request).next().map(this::getInstanceResponse); } - private Response getInstanceResponse(List instances) { + Response getInstanceResponse(List instances) { if (instances.isEmpty()) { - log.warn("No servers available for service: " + this.serviceId); + if (log.isWarnEnabled()) { + log.warn("No servers available for service: " + serviceId); + } return new EmptyResponse(); } // TODO: enforce order? diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplier.java index 5f9c46e53..c444ddd0a 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplier.java @@ -24,6 +24,7 @@ import org.springframework.cloud.client.DefaultServiceInstance; import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.core.env.Environment; import static org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory.PROPERTY_NAME; @@ -38,6 +39,10 @@ public interface ServiceInstanceListSupplier extends Supplier> get(Request request) { + return get(); + } + static ServiceInstanceListSupplierBuilder builder() { return new ServiceInstanceListSupplierBuilder(); } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplierBuilder.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplierBuilder.java index 59626774a..22cd7ac29 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplierBuilder.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSupplierBuilder.java @@ -172,6 +172,12 @@ public ServiceInstanceListSupplierBuilder withCaching() { return this; } + public ServiceInstanceListSupplierBuilder withRetryAwareness() { + DelegateCreator creator = (context, delegate) -> new RetryAwareServiceInstanceListSupplier(delegate); + creators.add(creator); + return this; + } + /** * Builds the {@link ServiceInstanceListSupplier} hierarchy. * @param context application context diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/support/ServiceInstanceListSuppliers.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/support/ServiceInstanceListSuppliers.java index 23eb359cd..5ac588fca 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/support/ServiceInstanceListSuppliers.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/support/ServiceInstanceListSuppliers.java @@ -26,7 +26,7 @@ import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; /** - * Utility class for service instances. + * Utility class for service instance list suppliers. * * @author Spencer Gibb * @author Olga Maciaszek-Sharma diff --git a/spring-cloud-loadbalancer/src/main/resources/META-INF/additional-spring-configuration-metadata.json b/spring-cloud-loadbalancer/src/main/resources/META-INF/additional-spring-configuration-metadata.json index 20fb59557..fb7d96b57 100644 --- a/spring-cloud-loadbalancer/src/main/resources/META-INF/additional-spring-configuration-metadata.json +++ b/spring-cloud-loadbalancer/src/main/resources/META-INF/additional-spring-configuration-metadata.json @@ -21,6 +21,12 @@ "name": "spring.cloud.loadbalancer.cache.enabled", "description": "Enables Spring Cloud LoadBalancer caching mechanism.", "type": "java.lang.Boolean" + }, + { + "defaultValue": true, + "name": "spring.cloud.loadbalancer.retry.avoid-previous-instance", + "description": "Enables wrapping ServiceInstanceListSupplier beans with `RetryAwareServiceInstanceListSupplier` if Spring-Retry is in the classpath.", + "type": "java.lang.Boolean" } ] } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfigurationTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfigurationTests.java index 7ef9b190d..4ab214662 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfigurationTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfigurationTests.java @@ -19,6 +19,7 @@ import org.junit.jupiter.api.Test; import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.test.context.FilteredClassLoader; import org.springframework.boot.test.context.runner.ApplicationContextRunner; import org.springframework.cloud.client.discovery.composite.CompositeDiscoveryClientAutoConfiguration; import org.springframework.cloud.client.discovery.composite.reactive.ReactiveCompositeDiscoveryClientAutoConfiguration; @@ -29,10 +30,12 @@ import org.springframework.cloud.loadbalancer.core.DelegatingServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.DiscoveryClientServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.HealthCheckServiceInstanceListSupplier; +import org.springframework.cloud.loadbalancer.core.RetryAwareServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.ZonePreferenceServiceInstanceListSupplier; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.retry.support.RetryTemplate; import org.springframework.web.reactive.function.client.WebClient; import static org.assertj.core.api.BDDAssertions.then; @@ -50,6 +53,12 @@ class LoadBalancerClientConfigurationTests { LoadBalancerClientConfiguration.class)); ApplicationContextRunner blockingDiscoveryClientRunner = new ApplicationContextRunner() + .withClassLoader(new FilteredClassLoader(RetryTemplate.class)) + .withConfiguration(AutoConfigurations.of(CompositeDiscoveryClientAutoConfiguration.class, + LoadBalancerCacheAutoConfiguration.class, LoadBalancerAutoConfiguration.class, + LoadBalancerClientConfiguration.class)); + + ApplicationContextRunner blockingDiscoveryClientRunnerWithRetry = new ApplicationContextRunner() .withConfiguration(AutoConfigurations.of(CompositeDiscoveryClientAutoConfiguration.class, LoadBalancerCacheAutoConfiguration.class, LoadBalancerAutoConfiguration.class, LoadBalancerClientConfiguration.class)); @@ -126,6 +135,30 @@ void shouldInstantiateDefaultBlockingServiceInstanceListSupplier() { }); } + @Test + void shouldWrapWithRetryAwareSupplierWhenRetryTemplateOnClasspath() { + blockingDiscoveryClientRunnerWithRetry.run(context -> { + ServiceInstanceListSupplier supplier = context.getBean(ServiceInstanceListSupplier.class); + then(supplier).isInstanceOf(RetryAwareServiceInstanceListSupplier.class); + then(((DelegatingServiceInstanceListSupplier) supplier).getDelegate()) + .isInstanceOf(CachingServiceInstanceListSupplier.class); + then(((DelegatingServiceInstanceListSupplier) ((DelegatingServiceInstanceListSupplier) supplier) + .getDelegate()).getDelegate()).isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + }); + } + + @Test + void shouldNotWrapWithRetryAwareSupplierWhenRetryTemplateOnClasspath() { + blockingDiscoveryClientRunner.withPropertyValues("spring.cloud.loadbalancer.retry.avoidPreviousInstance=false") + .run(context -> { + ServiceInstanceListSupplier supplier = context.getBean(ServiceInstanceListSupplier.class); + then(supplier).isInstanceOf(CachingServiceInstanceListSupplier.class); + then(((DelegatingServiceInstanceListSupplier) supplier).getDelegate()) + .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + }); + + } + @Configuration protected static class TestConfig { diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicyTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicyTests.java index 0c4f9670b..2d1900ad5 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicyTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicyTests.java @@ -23,7 +23,7 @@ import org.junit.jupiter.api.Test; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext; -import org.springframework.cloud.client.loadbalancer.LoadBalancerRetryProperties; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; import org.springframework.http.HttpMethod; import org.springframework.http.HttpRequest; @@ -47,7 +47,7 @@ class BlockingLoadBalancedRetryPolicyTests { private final LoadBalancedRetryContext context = mock(LoadBalancedRetryContext.class); - private final LoadBalancerRetryProperties retryProperties = new LoadBalancerRetryProperties(); + private final LoadBalancerProperties properties = new LoadBalancerProperties(); private final UnsupportedOperationException exception = new UnsupportedOperationException(); @@ -59,8 +59,8 @@ void setUp() { @Test void shouldExecuteIndicatedNumberOfSameAndNextInstanceRetriesAndCloseRetryContext() { - retryProperties.setMaxRetriesOnSameServiceInstance(1); - BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(retryProperties); + properties.getRetry().setMaxRetriesOnSameServiceInstance(1); + BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(properties); assertThat(retryPolicy.canRetrySameServer(context)).isTrue(); assertThat(retryPolicy.canRetryNextServer(context)).isTrue(); @@ -88,7 +88,7 @@ void shouldExecuteIndicatedNumberOfSameAndNextInstanceRetriesAndCloseRetryContex void shouldNotRetryWhenMethodNotGet() { when(httpRequest.getMethod()).thenReturn(HttpMethod.POST); when(context.getRequest()).thenReturn(httpRequest); - BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(retryProperties); + BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(properties); boolean canRetry = retryPolicy.canRetry(context); @@ -99,8 +99,8 @@ void shouldNotRetryWhenMethodNotGet() { void shouldRetryOnPostWhenEnabled() { when(httpRequest.getMethod()).thenReturn(HttpMethod.POST); when(context.getRequest()).thenReturn(httpRequest); - retryProperties.setRetryOnAllOperations(true); - BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(retryProperties); + properties.getRetry().setRetryOnAllOperations(true); + BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(properties); boolean canRetry = retryPolicy.canRetry(context); @@ -109,16 +109,16 @@ void shouldRetryOnPostWhenEnabled() { @Test void shouldResolveRetryableStatusCode() { - retryProperties.setRetryableStatusCodes(new HashSet<>(Arrays.asList(404, 502))); - BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(retryProperties); + properties.getRetry().setRetryableStatusCodes(new HashSet<>(Arrays.asList(404, 502))); + BlockingLoadBalancedRetryPolicy retryPolicy = getRetryPolicy(properties); boolean retryableStatusCode = retryPolicy.retryableStatusCode(404); assertThat(retryableStatusCode).isTrue(); } - private BlockingLoadBalancedRetryPolicy getRetryPolicy(LoadBalancerRetryProperties retryProperties) { - return new BlockingLoadBalancedRetryPolicy("test", loadBalancerClient, retryProperties); + private BlockingLoadBalancedRetryPolicy getRetryPolicy(LoadBalancerProperties properties) { + return new BlockingLoadBalancedRetryPolicy(properties); } } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplierTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplierTests.java new file mode 100644 index 000000000..b88826303 --- /dev/null +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplierTests.java @@ -0,0 +1,81 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.loadbalancer.core; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.DefaultRequest; +import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; +import org.springframework.cloud.loadbalancer.support.ServiceInstanceListSuppliers; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link RetryAwareServiceInstanceListSupplier}. + * + * @author Olga Maciaszek-Sharma + */ +class RetryAwareServiceInstanceListSupplierTests { + + private final String serviceId = "test"; + + private static DefaultServiceInstance instance(String serviceId, String host, boolean secure) { + return new DefaultServiceInstance(serviceId, serviceId, host, 80, secure); + } + + @Test + void shouldReturnEmptyListIfNoInstances() { + ServiceInstanceListSupplier delegate = ServiceInstanceListSuppliers.from(serviceId); + ServiceInstanceListSupplier supplier = new RetryAwareServiceInstanceListSupplier(delegate); + + List returnedInstances = supplier.get(new DefaultRequest<>(new RetryableRequestContext(null))) + .blockFirst(); + + assertThat(returnedInstances).isEmpty(); + } + + @Test + void shouldReturnFilteredInstances() { + ServiceInstance firstInstance = instance(serviceId, "1host", false); + ServiceInstance secondInstance = instance(serviceId, "2host-secure", true); + ServiceInstanceListSupplier delegate = ServiceInstanceListSuppliers.from(serviceId, firstInstance, + secondInstance); + ServiceInstanceListSupplier supplier = new RetryAwareServiceInstanceListSupplier(delegate); + + List returnedInstances = supplier + .get(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).blockFirst(); + + assertThat(returnedInstances).containsExactly(secondInstance); + } + + @Test + void shouldReturnAllInstancesIfFilteredInstancesEmpty() { + ServiceInstance firstInstance = instance(serviceId, "1host", false); + ServiceInstanceListSupplier delegate = ServiceInstanceListSuppliers.from(serviceId, firstInstance); + ServiceInstanceListSupplier supplier = new RetryAwareServiceInstanceListSupplier(delegate); + + List returnedInstances = supplier + .get(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).blockFirst(); + + assertThat(returnedInstances).containsExactly(firstInstance); + } + +}