From c5c366bebf4f8e22d6878cec3e7d83c2b9a6a344 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 24 Sep 2020 14:26:19 +0200 Subject: [PATCH 01/13] cherry-pick switching to properties --- .../LoadBalancerAutoConfiguration.java | 10 +- .../LoadBalancerRetryProperties.java | 105 ------------------ .../RetryLoadBalancerInterceptor.java | 11 +- .../reactive/LoadBalancerProperties.java | 90 +++++++++++++++ ...actLoadBalancerAutoConfigurationTests.java | 6 - ...ancerRequestFactoryConfigurationTests.java | 6 - .../RetryLoadBalancerInterceptorTests.java | 74 ++++++------ .../BlockingLoadBalancedRetryFactory.java | 10 +- .../BlockingLoadBalancedRetryPolicy.java | 20 ++-- ...ngLoadBalancerClientAutoConfiguration.java | 7 +- .../BlockingLoadBalancedRetryPolicyTests.java | 22 ++-- 11 files changed, 164 insertions(+), 197 deletions(-) delete mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRetryProperties.java 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..f0e4c41f8 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 @@ -48,7 +48,7 @@ @Configuration(proxyBeanMethods = false) @ConditionalOnClass(RestTemplate.class) @ConditionalOnBean(LoadBalancerClient.class) -@EnableConfigurationProperties(LoadBalancerRetryProperties.class) +@EnableConfigurationProperties(LoadBalancerProperties.class) public class LoadBalancerAutoConfiguration { @LoadBalanced @@ -125,11 +125,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..e1505f489 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; } @@ -127,7 +124,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/reactive/LoadBalancerProperties.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerProperties.java index 92a46fbf1..b8a74301b 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,8 @@ public class LoadBalancerProperties { */ private Map hint = new LinkedCaseInsensitiveMap<>(); + private Retry retry = new Retry(); + public HealthCheck getHealthCheck() { return healthCheck; } @@ -59,6 +64,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 +112,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/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/blocking/retry/BlockingLoadBalancedRetryFactory.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryFactory.java index ecf995202..86ccfeb43 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(serviceId, serviceInstanceChooser, 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..3e50838fd 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,8 @@ 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,10 +33,10 @@ */ public class BlockingLoadBalancedRetryPolicy implements LoadBalancedRetryPolicy { - private final LoadBalancerRetryProperties retryProperties; - private final ServiceInstanceChooser serviceInstanceChooser; + private final LoadBalancerProperties loadBalancerProperties; + private final String serviceId; private int sameServerCount = 0; @@ -44,26 +44,28 @@ public class BlockingLoadBalancedRetryPolicy implements LoadBalancedRetryPolicy private int nextServerCount = 0; public BlockingLoadBalancedRetryPolicy(String serviceId, ServiceInstanceChooser serviceInstanceChooser, - LoadBalancerRetryProperties retryProperties) { + LoadBalancerProperties loadBalancerProperties) { this.serviceId = serviceId; this.serviceInstanceChooser = serviceInstanceChooser; - this.retryProperties = retryProperties; + this.loadBalancerProperties = loadBalancerProperties; } public boolean canRetry(LoadBalancedRetryContext context) { HttpMethod method = context.getRequest().getMethod(); - return HttpMethod.GET.equals(method) || retryProperties.isRetryOnAllOperations(); + return HttpMethod.GET.equals(method) || loadBalancerProperties.getRetry().isRetryOnAllOperations(); } @Override public boolean canRetrySameServer(LoadBalancedRetryContext context) { - return sameServerCount < retryProperties.getMaxRetriesOnSameServiceInstance() && canRetry(context); + return sameServerCount < loadBalancerProperties.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 <= loadBalancerProperties.getRetry().getMaxRetriesOnNextServiceInstance() + && canRetry(context); } @Override @@ -91,7 +93,7 @@ public void registerThrowable(LoadBalancedRetryContext context, Throwable throwa @Override public boolean retryableStatusCode(int statusCode) { - return retryProperties.getRetryableStatusCodes().contains(statusCode); + return loadBalancerProperties.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/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..3d7b8710b 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("test", loadBalancerClient, properties); } } From cd6335d009aee1b89f76aec7e8590126c4b026c3 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 24 Sep 2020 20:17:18 +0200 Subject: [PATCH 02/13] Pass information on previous ServiceInstance to RequestContext. # Conflicts: # spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicy.java --- .../loadbalancer/InterceptorRetryPolicy.java | 2 +- .../LoadBalancedRetryContext.java | 11 +++++ .../RetryLoadBalancerInterceptor.java | 16 +++++-- .../loadbalancer/RetryableRequestContext.java | 46 +++++++++++++++++++ .../InterceptorRetryPolicyTest.java | 5 +- .../BlockingLoadBalancedRetryFactory.java | 2 +- .../BlockingLoadBalancedRetryPolicy.java | 29 +++++------- .../BlockingLoadBalancedRetryPolicyTests.java | 2 +- 8 files changed, 85 insertions(+), 28 deletions(-) create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java 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..9c83b0e1e 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 @@ -56,7 +56,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..1e598eaeb 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 @@ -32,6 +32,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 +73,16 @@ public ServiceInstance getServiceInstance() { * @param serviceInstance The service instance to use during the retry. */ public void setServiceInstance(ServiceInstance serviceInstance) { + previousServiceInstance = this.serviceInstance; this.serviceInstance = serviceInstance; } + public ServiceInstance getPreviousServiceInstance() { + return previousServiceInstance; + } + + void setPreviousServiceInstance(ServiceInstance previousServiceInstance) { + this.previousServiceInstance = previousServiceInstance; + } + } 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 e1505f489..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 @@ -82,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) { 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..cb3d5d831 --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java @@ -0,0 +1,46 @@ +/* + * 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; + +/** + * @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/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..846db1e8b 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; @@ -69,10 +68,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-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 86ccfeb43..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 @@ -39,7 +39,7 @@ public BlockingLoadBalancedRetryFactory(LoadBalancerProperties loadBalancerPrope @Override public LoadBalancedRetryPolicy createRetryPolicy(String serviceId, ServiceInstanceChooser serviceInstanceChooser) { - return new BlockingLoadBalancedRetryPolicy(serviceId, serviceInstanceChooser, loadBalancerProperties); + 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 3e50838fd..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,7 +18,6 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy; -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,39 +32,30 @@ */ public class BlockingLoadBalancedRetryPolicy implements LoadBalancedRetryPolicy { - private final ServiceInstanceChooser serviceInstanceChooser; - - private final LoadBalancerProperties loadBalancerProperties; - - private final String serviceId; + private final LoadBalancerProperties properties; private int sameServerCount = 0; private int nextServerCount = 0; - public BlockingLoadBalancedRetryPolicy(String serviceId, ServiceInstanceChooser serviceInstanceChooser, - LoadBalancerProperties loadBalancerProperties) { - this.serviceId = serviceId; - this.serviceInstanceChooser = serviceInstanceChooser; - this.loadBalancerProperties = loadBalancerProperties; + public BlockingLoadBalancedRetryPolicy(LoadBalancerProperties properties) { + this.properties = properties; } public boolean canRetry(LoadBalancedRetryContext context) { HttpMethod method = context.getRequest().getMethod(); - return HttpMethod.GET.equals(method) || loadBalancerProperties.getRetry().isRetryOnAllOperations(); + return HttpMethod.GET.equals(method) || properties.getRetry().isRetryOnAllOperations(); } @Override public boolean canRetrySameServer(LoadBalancedRetryContext context) { - return sameServerCount < loadBalancerProperties.getRetry().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 <= loadBalancerProperties.getRetry().getMaxRetriesOnNextServiceInstance() - && canRetry(context); + return nextServerCount <= properties.getRetry().getMaxRetriesOnNextServiceInstance() && canRetry(context); } @Override @@ -83,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 { @@ -93,7 +86,7 @@ public void registerThrowable(LoadBalancedRetryContext context, Throwable throwa @Override public boolean retryableStatusCode(int statusCode) { - return loadBalancerProperties.getRetry().getRetryableStatusCodes().contains(statusCode); + return properties.getRetry().getRetryableStatusCodes().contains(statusCode); } } 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 3d7b8710b..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 @@ -118,7 +118,7 @@ void shouldResolveRetryableStatusCode() { } private BlockingLoadBalancedRetryPolicy getRetryPolicy(LoadBalancerProperties properties) { - return new BlockingLoadBalancedRetryPolicy("test", loadBalancerClient, properties); + return new BlockingLoadBalancedRetryPolicy(properties); } } From 80087fe14471efd1cf0fea8006058edb930e5836 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 25 Sep 2020 15:38:44 +0200 Subject: [PATCH 03/13] Add a RoundRobinLoadBalancer implementation that avoids same service instance while retrying. --- ...reviousInstanceRoundRobinLoadBalancer.java | 99 +++++++++++++++++++ .../core/RoundRobinLoadBalancer.java | 12 ++- 2 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java new file mode 100644 index 000000000..3d0924f71 --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java @@ -0,0 +1,99 @@ +/* + * 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.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import reactor.core.publisher.Mono; + +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.DefaultResponse; +import org.springframework.cloud.client.loadbalancer.EmptyResponse; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.client.loadbalancer.RetryLoadBalancerInterceptor; +import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; + +/** + * A modified {@link RoundRobinLoadBalancer} implementation that avoids picking the same + * service instance while retrying requests. + * + * @author Olga Maciaszek-Sharma + * @see RetryLoadBalancerInterceptor + */ +public class AvoidPreviousInstanceRoundRobinLoadBalancer extends RoundRobinLoadBalancer { + + private static final Log LOG = LogFactory.getLog(AvoidPreviousInstanceRoundRobinLoadBalancer.class); + + public AvoidPreviousInstanceRoundRobinLoadBalancer( + ObjectProvider serviceInstanceListSupplierProvider, String serviceId) { + super(serviceInstanceListSupplierProvider, serviceId); + } + + public AvoidPreviousInstanceRoundRobinLoadBalancer( + ObjectProvider serviceInstanceListSupplierProvider, String serviceId, + int seedPosition) { + super(serviceInstanceListSupplierProvider, serviceId, seedPosition); + } + + @Override + // see original + // https://github.com/Netflix/ocelli/blob/master/ocelli-core/ + // src/main/java/netflix/ocelli/loadbalancer/RoundRobinLoadBalancer.java + public Mono> choose(Request request) { + if (!(request.getContext() instanceof RetryableRequestContext)) { + return super.choose(request); + } + RetryableRequestContext context = (RetryableRequestContext) request.getContext(); + + ServiceInstanceListSupplier supplier = serviceInstanceListSupplierProvider + .getIfAvailable(NoopServiceInstanceListSupplier::new); + return supplier.get().next() + .map(instances -> getInstanceResponse(instances, context.getPreviousServiceInstance())); + } + + Response getInstanceResponse(List instances, + ServiceInstance previousServiceInstance) { + if (previousServiceInstance == null) { + return super.getInstanceResponse(instances); + } + if (instances.isEmpty()) { + if (LOG.isWarnEnabled()) { + LOG.warn("No servers available for service: " + serviceId); + } + return new EmptyResponse(); + } + instances.remove(previousServiceInstance); + if (instances.isEmpty()) { + if (LOG.isWarnEnabled()) { + LOG.warn("No other instance available, returning previous instance: " + previousServiceInstance); + } + return new DefaultResponse(previousServiceInstance); + } + + // TODO: enforce order? + int pos = Math.abs(this.position.incrementAndGet()); + + ServiceInstance instance = instances.get(pos % instances.size()); + + return new DefaultResponse(instance); + } + +} 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..050d36529 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 @@ -81,9 +81,11 @@ public Mono> choose(Request request) { return supplier.get().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? From ffa65fa20ee53b1d90c7a1c7ce84cc6ba94b097f Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 25 Sep 2020 16:45:29 +0200 Subject: [PATCH 04/13] Wrap instances in ArrayList. Add tests. --- ...reviousInstanceRoundRobinLoadBalancer.java | 8 ++- .../support/ServiceInstanceListSuppliers.java | 2 +- ...usInstanceRoundRobinLoadBalancerTests.java | 71 +++++++++++++++++++ 3 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java index 3d0924f71..4b8ef1fb1 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java @@ -16,6 +16,7 @@ package org.springframework.cloud.loadbalancer.core; +import java.util.ArrayList; import java.util.List; import org.apache.commons.logging.Log; @@ -80,8 +81,9 @@ Response getInstanceResponse(List instances, } return new EmptyResponse(); } - instances.remove(previousServiceInstance); - if (instances.isEmpty()) { + List finalInstances = new ArrayList<>(instances); + finalInstances.remove(previousServiceInstance); + if (finalInstances.isEmpty()) { if (LOG.isWarnEnabled()) { LOG.warn("No other instance available, returning previous instance: " + previousServiceInstance); } @@ -91,7 +93,7 @@ Response getInstanceResponse(List instances, // TODO: enforce order? int pos = Math.abs(this.position.incrementAndGet()); - ServiceInstance instance = instances.get(pos % instances.size()); + ServiceInstance instance = finalInstances.get(pos % finalInstances.size()); return new DefaultResponse(instance); } 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/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java new file mode 100644 index 000000000..00608ee38 --- /dev/null +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java @@ -0,0 +1,71 @@ +package org.springframework.cloud.loadbalancer.core; + +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.EmptyResponse; +import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; +import org.springframework.cloud.loadbalancer.support.ServiceInstanceListSuppliers; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Olga Maciaszek-Sharma + */ +class AvoidPreviousInstanceRoundRobinLoadBalancerTests { + + 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 shouldReturnEmptyResponseIfNoInstances() { + AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( + ServiceInstanceListSuppliers.toProvider(serviceId), serviceId); + + Response response = loadBalancer + .choose(new DefaultRequest<>(new RetryableRequestContext(null))) + .block(); + + assertThat(response).isInstanceOf(EmptyResponse.class); + } + + @Test + void shouldReturnADifferentInstance() { + ServiceInstance firstInstance = instance(serviceId, "1host", false); + ServiceInstance secondInstance = instance(serviceId, "2host-secure", true); + AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( + ServiceInstanceListSuppliers + .toProvider(serviceId, firstInstance, secondInstance), serviceId); + + Response firstResponse = loadBalancer + .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))) + .block(); + Response secondResponse = loadBalancer + .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))) + .block(); + + assertThat(firstResponse.getServer()).isEqualTo(secondInstance); + assertThat(secondResponse.getServer()).isEqualTo(secondInstance); + } + + @Test + void shouldReturnSameInstanceIfDifferentOneNotAvailable() { + ServiceInstance firstInstance = instance(serviceId, "1host", false); + AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( + ServiceInstanceListSuppliers + .toProvider(serviceId, firstInstance), serviceId); + + Response firstResponse = loadBalancer + .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))) + .block(); + + assertThat(firstResponse.getServer()).isEqualTo(firstInstance); + } + +} \ No newline at end of file From cc8b65dfe63aed279bffe74f339635734c341d7a Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 28 Sep 2020 12:34:11 +0200 Subject: [PATCH 05/13] Enable AvoidPreviousInstanceRoundRobinLoadBalancer by default if SpringRetry on classpath. --- .../LoadBalancerClientConfiguration.java | 15 ++++++++ ...itional-spring-configuration-metadata.json | 6 ++++ ...usInstanceRoundRobinLoadBalancerTests.java | 36 ++++++++++++------- 3 files changed, 44 insertions(+), 13 deletions(-) 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..2858aacf4 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 @@ -17,6 +17,7 @@ package org.springframework.cloud.loadbalancer.annotation; 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; @@ -25,6 +26,7 @@ import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; +import org.springframework.cloud.loadbalancer.core.AvoidPreviousInstanceRoundRobinLoadBalancer; import org.springframework.cloud.loadbalancer.core.ReactorLoadBalancer; import org.springframework.cloud.loadbalancer.core.RoundRobinLoadBalancer; import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; @@ -32,6 +34,7 @@ import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Primary; import org.springframework.core.annotation.Order; import org.springframework.core.env.Environment; @@ -55,6 +58,18 @@ public ReactorLoadBalancer reactorServiceInstanceLoadBalancer(E loadBalancerClientFactory.getLazyProvider(name, ServiceInstanceListSupplier.class), name); } + @Bean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnProperty(name = "org.springframework.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", + havingValue = "true", matchIfMissing = true) + @Primary + public ReactorLoadBalancer avoidRetryingOnSameServiceInstanceLoadBalancer(Environment environment, + LoadBalancerClientFactory loadBalancerClientFactory) { + String name = environment.getProperty(LoadBalancerClientFactory.PROPERTY_NAME); + return new AvoidPreviousInstanceRoundRobinLoadBalancer( + loadBalancerClientFactory.getLazyProvider(name, ServiceInstanceListSupplier.class), name); + } + @Configuration(proxyBeanMethods = false) @ConditionalOnReactiveDiscoveryEnabled @Order(REACTIVE_SERVICE_INSTANCE_SUPPLIER_ORDER) 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..92ec25807 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": "org.springframework.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", + "description": "Instantiates AvoidPreviousInstanceRoundRobinLoadBalancer if Spring-Retry is in the classpath.", + "type": "java.lang.Boolean" } ] } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java index 00608ee38..374f594e5 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java @@ -1,3 +1,19 @@ +/* + * 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 org.junit.jupiter.api.Test; @@ -29,8 +45,7 @@ void shouldReturnEmptyResponseIfNoInstances() { ServiceInstanceListSuppliers.toProvider(serviceId), serviceId); Response response = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(null))) - .block(); + .choose(new DefaultRequest<>(new RetryableRequestContext(null))).block(); assertThat(response).isInstanceOf(EmptyResponse.class); } @@ -40,15 +55,12 @@ void shouldReturnADifferentInstance() { ServiceInstance firstInstance = instance(serviceId, "1host", false); ServiceInstance secondInstance = instance(serviceId, "2host-secure", true); AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( - ServiceInstanceListSuppliers - .toProvider(serviceId, firstInstance, secondInstance), serviceId); + ServiceInstanceListSuppliers.toProvider(serviceId, firstInstance, secondInstance), serviceId); Response firstResponse = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))) - .block(); + .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).block(); Response secondResponse = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))) - .block(); + .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).block(); assertThat(firstResponse.getServer()).isEqualTo(secondInstance); assertThat(secondResponse.getServer()).isEqualTo(secondInstance); @@ -58,14 +70,12 @@ void shouldReturnADifferentInstance() { void shouldReturnSameInstanceIfDifferentOneNotAvailable() { ServiceInstance firstInstance = instance(serviceId, "1host", false); AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( - ServiceInstanceListSuppliers - .toProvider(serviceId, firstInstance), serviceId); + ServiceInstanceListSuppliers.toProvider(serviceId, firstInstance), serviceId); Response firstResponse = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))) - .block(); + .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).block(); assertThat(firstResponse.getServer()).isEqualTo(firstInstance); } -} \ No newline at end of file +} From 45f45e64738a55f49b624224a8d4ea5d65a214a4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 28 Sep 2020 13:31:40 +0200 Subject: [PATCH 06/13] Fix failing tests. Add javadocs and author tags. --- .../cloud/client/loadbalancer/InterceptorRetryPolicy.java | 1 + .../cloud/client/loadbalancer/LoadBalancedRetryContext.java | 5 +++-- .../client/loadbalancer/LoadBalancerAutoConfiguration.java | 1 + .../cloud/client/loadbalancer/RetryableRequestContext.java | 3 +++ .../client/loadbalancer/reactive/LoadBalancerProperties.java | 3 +++ .../client/loadbalancer/InterceptorRetryPolicyTest.java | 1 + .../core/AvoidPreviousInstanceRoundRobinLoadBalancer.java | 1 + .../blocking/client/BlockingLoadBalancerClientTests.java | 2 +- .../AvoidPreviousInstanceRoundRobinLoadBalancerTests.java | 2 ++ 9 files changed, 16 insertions(+), 3 deletions(-) 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 9c83b0e1e..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 { 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 1e598eaeb..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 { @@ -73,7 +74,7 @@ public ServiceInstance getServiceInstance() { * @param serviceInstance The service instance to use during the retry. */ public void setServiceInstance(ServiceInstance serviceInstance) { - previousServiceInstance = this.serviceInstance; + setPreviousServiceInstance(this.serviceInstance); this.serviceInstance = serviceInstance; } @@ -81,7 +82,7 @@ public ServiceInstance getPreviousServiceInstance() { return previousServiceInstance; } - void setPreviousServiceInstance(ServiceInstance 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 f0e4c41f8..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,6 +44,7 @@ * @author Dave Syer * @author Will Tran * @author Gang Li + * @author Olga Maciaszek-Sharma */ @Configuration(proxyBeanMethods = false) @ConditionalOnClass(RestTemplate.class) 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 index cb3d5d831..20595fa29 100644 --- 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 @@ -19,6 +19,9 @@ 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 { 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 b8a74301b..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 @@ -46,6 +46,9 @@ 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() { 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 846db1e8b..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 @@ -34,6 +34,7 @@ /** * @author Ryan Baxter + * @author Olga Maciaszek-Sharma */ @RunWith(MockitoJUnitRunner.class) public class InterceptorRetryPolicyTest { diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java index 4b8ef1fb1..a87d51d1d 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java @@ -37,6 +37,7 @@ * service instance while retrying requests. * * @author Olga Maciaszek-Sharma + * @since 3.0.0 * @see RetryLoadBalancerInterceptor */ public class AvoidPreviousInstanceRoundRobinLoadBalancer extends RoundRobinLoadBalancer { diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java index 251dccdba..f0a181c09 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java @@ -61,7 +61,7 @@ * * @author Olga Maciaszek-Sharma */ -@SpringBootTest +@SpringBootTest(properties = "org.springframework.cloud.loadbalancer.retry.avoidPreviousInstance.enabled=false") class BlockingLoadBalancerClientTests { @Autowired diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java index 374f594e5..4fccb6af4 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java @@ -29,6 +29,8 @@ import static org.assertj.core.api.Assertions.assertThat; /** + * Tests for {@link AvoidPreviousInstanceRoundRobinLoadBalancer}. + * * @author Olga Maciaszek-Sharma */ class AvoidPreviousInstanceRoundRobinLoadBalancerTests { From f5c9fee0b1f7ffa31fdc0101881f946fb6192ca4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 28 Sep 2020 13:47:10 +0200 Subject: [PATCH 07/13] Fix properties. --- .../LoadBalancerClientConfiguration.java | 25 +++++++++++++++++-- ...itional-spring-configuration-metadata.json | 2 +- .../BlockingLoadBalancerClientTests.java | 2 +- 3 files changed, 25 insertions(+), 4 deletions(-) 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 2858aacf4..b31dc9f42 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,6 +16,7 @@ package org.springframework.cloud.loadbalancer.annotation; +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; @@ -33,6 +34,7 @@ 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; @@ -60,8 +62,7 @@ public ReactorLoadBalancer reactorServiceInstanceLoadBalancer(E @Bean @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") - @ConditionalOnProperty(name = "org.springframework.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", - havingValue = "true", matchIfMissing = true) + @Conditional(OnAvoidPreviousInstanceAndRetryEnabledCondition.class) @Primary public ReactorLoadBalancer avoidRetryingOnSameServiceInstanceLoadBalancer(Environment environment, LoadBalancerClientFactory loadBalancerClientFactory) { @@ -144,4 +145,24 @@ public ServiceInstanceListSupplier healthCheckDiscoveryClientServiceInstanceList } + 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.avoidPreviousInstance.enabled", + havingValue = "true", matchIfMissing = true) + static class AvoidPreviousInstanceEnabled { + + } + + } + } 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 92ec25807..12140dd2b 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 @@ -24,7 +24,7 @@ }, { "defaultValue": true, - "name": "org.springframework.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", + "name": "spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", "description": "Instantiates AvoidPreviousInstanceRoundRobinLoadBalancer if Spring-Retry is in the classpath.", "type": "java.lang.Boolean" } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java index f0a181c09..0ccbf0fb7 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java @@ -61,7 +61,7 @@ * * @author Olga Maciaszek-Sharma */ -@SpringBootTest(properties = "org.springframework.cloud.loadbalancer.retry.avoidPreviousInstance.enabled=false") +@SpringBootTest(properties = "spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled") class BlockingLoadBalancerClientTests { @Autowired From c71c6706153153574d686ccaeda0822253065ff7 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 28 Sep 2020 13:55:23 +0200 Subject: [PATCH 08/13] Add documentation. --- docs/src/main/asciidoc/spring-cloud-commons.adoc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 2ae870414..584bce0a8 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 instantiate `AvoidPreviousInstanceRoundRobinLoadBalancer` that will select a different instance from the one previously chosen if available. You can switch back to using the basic `RoundRobinLoadBalancer` by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled` to `false`. + ==== [source,java,indent=0] ---- From 3a5df47557d097209e92fe23250eef482f126dfc Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 28 Sep 2020 18:32:54 +0200 Subject: [PATCH 09/13] Fix docs after review. --- docs/src/main/asciidoc/spring-cloud-commons.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 584bce0a8..5725ccd24 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -449,7 +449,7 @@ You can set: - `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 instantiate `AvoidPreviousInstanceRoundRobinLoadBalancer` that will select a different instance from the one previously chosen if available. You can switch back to using the basic `RoundRobinLoadBalancer` by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled` to `false`. +NOTE: For load-balanced retries, by default, we instantiate `AvoidPreviousInstanceRoundRobinLoadBalancer` to select a different instance from the one previously chosen, if available. You can switch back to using the basic `RoundRobinLoadBalancer` by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled` to `false`. ==== [source,java,indent=0] From fd013b46e9351dc5ff83ab6a22f9757d37d40a48 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 28 Sep 2020 18:35:04 +0200 Subject: [PATCH 10/13] Fix docs after review. --- docs/src/main/asciidoc/_configprops.adoc | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/src/main/asciidoc/_configprops.adoc b/docs/src/main/asciidoc/_configprops.adoc index b046388d1..29f910027 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.avoidPreviousInstance.enabled | true | Instantiates AvoidPreviousInstanceRoundRobinLoadBalancer 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. From d6124369aa5ab28c3bf1d574b20cbc74f608c931 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 29 Sep 2020 19:10:28 +0200 Subject: [PATCH 11/13] Handle avoiding previous instance with ServiceInstanceListSupplier in place of LoadBalancer. --- docs/src/main/asciidoc/_configprops.adoc | 2 +- .../main/asciidoc/spring-cloud-commons.adoc | 2 +- .../LoadBalancerClientConfiguration.java | 37 ++++--- ...reviousInstanceRoundRobinLoadBalancer.java | 102 ------------------ ...RetryAwareServiceInstanceListSupplier.java | 85 +++++++++++++++ .../core/RoundRobinLoadBalancer.java | 2 +- .../core/ServiceInstanceListSupplier.java | 5 + .../ServiceInstanceListSupplierBuilder.java | 6 ++ ...itional-spring-configuration-metadata.json | 4 +- .../LoadBalancerClientConfigurationTests.java | 33 ++++++ .../BlockingLoadBalancerClientTests.java | 2 +- ...usInstanceRoundRobinLoadBalancerTests.java | 83 -------------- ...AwareServiceInstanceListSupplierTests.java | 81 ++++++++++++++ 13 files changed, 239 insertions(+), 205 deletions(-) delete mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplier.java delete mode 100644 spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java create mode 100644 spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/RetryAwareServiceInstanceListSupplierTests.java diff --git a/docs/src/main/asciidoc/_configprops.adoc b/docs/src/main/asciidoc/_configprops.adoc index 29f910027..89aa68c23 100644 --- a/docs/src/main/asciidoc/_configprops.adoc +++ b/docs/src/main/asciidoc/_configprops.adoc @@ -35,7 +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.avoidPreviousInstance.enabled | true | Instantiates AvoidPreviousInstanceRoundRobinLoadBalancer if Spring-Retry is in the classpath. +|spring.cloud.loadbalancer.retry.avoidPreviousInstance | 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 5725ccd24..68a4669a2 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -449,7 +449,7 @@ You can set: - `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 instantiate `AvoidPreviousInstanceRoundRobinLoadBalancer` to select a different instance from the one previously chosen, if available. You can switch back to using the basic `RoundRobinLoadBalancer` by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled` to `false`. +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 behaviour by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance` to `false`. ==== [source,java,indent=0] 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 b31dc9f42..e5c10ac12 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,6 +16,7 @@ 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; @@ -27,8 +28,8 @@ import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; -import org.springframework.cloud.loadbalancer.core.AvoidPreviousInstanceRoundRobinLoadBalancer; 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; @@ -39,6 +40,7 @@ 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 @@ -60,17 +62,6 @@ public ReactorLoadBalancer reactorServiceInstanceLoadBalancer(E loadBalancerClientFactory.getLazyProvider(name, ServiceInstanceListSupplier.class), name); } - @Bean - @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") - @Conditional(OnAvoidPreviousInstanceAndRetryEnabledCondition.class) - @Primary - public ReactorLoadBalancer avoidRetryingOnSameServiceInstanceLoadBalancer(Environment environment, - LoadBalancerClientFactory loadBalancerClientFactory) { - String name = environment.getProperty(LoadBalancerClientFactory.PROPERTY_NAME); - return new AvoidPreviousInstanceRoundRobinLoadBalancer( - loadBalancerClientFactory.getLazyProvider(name, ServiceInstanceListSupplier.class), name); - } - @Configuration(proxyBeanMethods = false) @ConditionalOnReactiveDiscoveryEnabled @Order(REACTIVE_SERVICE_INSTANCE_SUPPLIER_ORDER) @@ -145,6 +136,24 @@ 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() { @@ -157,8 +166,8 @@ static class LoadBalancerRetryEnabled { } - @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", - havingValue = "true", matchIfMissing = true) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoidPreviousInstance", havingValue = "true", + matchIfMissing = true) static class AvoidPreviousInstanceEnabled { } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java deleted file mode 100644 index a87d51d1d..000000000 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancer.java +++ /dev/null @@ -1,102 +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.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.Mono; - -import org.springframework.beans.factory.ObjectProvider; -import org.springframework.cloud.client.ServiceInstance; -import org.springframework.cloud.client.loadbalancer.DefaultResponse; -import org.springframework.cloud.client.loadbalancer.EmptyResponse; -import org.springframework.cloud.client.loadbalancer.Request; -import org.springframework.cloud.client.loadbalancer.Response; -import org.springframework.cloud.client.loadbalancer.RetryLoadBalancerInterceptor; -import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; - -/** - * A modified {@link RoundRobinLoadBalancer} implementation that avoids picking the same - * service instance while retrying requests. - * - * @author Olga Maciaszek-Sharma - * @since 3.0.0 - * @see RetryLoadBalancerInterceptor - */ -public class AvoidPreviousInstanceRoundRobinLoadBalancer extends RoundRobinLoadBalancer { - - private static final Log LOG = LogFactory.getLog(AvoidPreviousInstanceRoundRobinLoadBalancer.class); - - public AvoidPreviousInstanceRoundRobinLoadBalancer( - ObjectProvider serviceInstanceListSupplierProvider, String serviceId) { - super(serviceInstanceListSupplierProvider, serviceId); - } - - public AvoidPreviousInstanceRoundRobinLoadBalancer( - ObjectProvider serviceInstanceListSupplierProvider, String serviceId, - int seedPosition) { - super(serviceInstanceListSupplierProvider, serviceId, seedPosition); - } - - @Override - // see original - // https://github.com/Netflix/ocelli/blob/master/ocelli-core/ - // src/main/java/netflix/ocelli/loadbalancer/RoundRobinLoadBalancer.java - public Mono> choose(Request request) { - if (!(request.getContext() instanceof RetryableRequestContext)) { - return super.choose(request); - } - RetryableRequestContext context = (RetryableRequestContext) request.getContext(); - - ServiceInstanceListSupplier supplier = serviceInstanceListSupplierProvider - .getIfAvailable(NoopServiceInstanceListSupplier::new); - return supplier.get().next() - .map(instances -> getInstanceResponse(instances, context.getPreviousServiceInstance())); - } - - Response getInstanceResponse(List instances, - ServiceInstance previousServiceInstance) { - if (previousServiceInstance == null) { - return super.getInstanceResponse(instances); - } - if (instances.isEmpty()) { - if (LOG.isWarnEnabled()) { - LOG.warn("No servers available for service: " + serviceId); - } - return new EmptyResponse(); - } - List finalInstances = new ArrayList<>(instances); - finalInstances.remove(previousServiceInstance); - if (finalInstances.isEmpty()) { - if (LOG.isWarnEnabled()) { - LOG.warn("No other instance available, returning previous instance: " + previousServiceInstance); - } - return new DefaultResponse(previousServiceInstance); - } - - // TODO: enforce order? - int pos = Math.abs(this.position.incrementAndGet()); - - ServiceInstance instance = finalInstances.get(pos % finalInstances.size()); - - return new DefaultResponse(instance); - } - -} 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 050d36529..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 @@ -78,7 +78,7 @@ 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); } Response getInstanceResponse(List instances) { 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/resources/META-INF/additional-spring-configuration-metadata.json b/spring-cloud-loadbalancer/src/main/resources/META-INF/additional-spring-configuration-metadata.json index 12140dd2b..a538cce69 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 @@ -24,8 +24,8 @@ }, { "defaultValue": true, - "name": "spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled", - "description": "Instantiates AvoidPreviousInstanceRoundRobinLoadBalancer if Spring-Retry is in the classpath.", + "name": "spring.cloud.loadbalancer.retry.avoidPreviousInstance", + "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/client/BlockingLoadBalancerClientTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java index 0ccbf0fb7..251dccdba 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java @@ -61,7 +61,7 @@ * * @author Olga Maciaszek-Sharma */ -@SpringBootTest(properties = "spring.cloud.loadbalancer.retry.avoidPreviousInstance.enabled") +@SpringBootTest class BlockingLoadBalancerClientTests { @Autowired diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java deleted file mode 100644 index 4fccb6af4..000000000 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/AvoidPreviousInstanceRoundRobinLoadBalancerTests.java +++ /dev/null @@ -1,83 +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.loadbalancer.core; - -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.EmptyResponse; -import org.springframework.cloud.client.loadbalancer.Response; -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 AvoidPreviousInstanceRoundRobinLoadBalancer}. - * - * @author Olga Maciaszek-Sharma - */ -class AvoidPreviousInstanceRoundRobinLoadBalancerTests { - - 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 shouldReturnEmptyResponseIfNoInstances() { - AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( - ServiceInstanceListSuppliers.toProvider(serviceId), serviceId); - - Response response = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(null))).block(); - - assertThat(response).isInstanceOf(EmptyResponse.class); - } - - @Test - void shouldReturnADifferentInstance() { - ServiceInstance firstInstance = instance(serviceId, "1host", false); - ServiceInstance secondInstance = instance(serviceId, "2host-secure", true); - AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( - ServiceInstanceListSuppliers.toProvider(serviceId, firstInstance, secondInstance), serviceId); - - Response firstResponse = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).block(); - Response secondResponse = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).block(); - - assertThat(firstResponse.getServer()).isEqualTo(secondInstance); - assertThat(secondResponse.getServer()).isEqualTo(secondInstance); - } - - @Test - void shouldReturnSameInstanceIfDifferentOneNotAvailable() { - ServiceInstance firstInstance = instance(serviceId, "1host", false); - AvoidPreviousInstanceRoundRobinLoadBalancer loadBalancer = new AvoidPreviousInstanceRoundRobinLoadBalancer( - ServiceInstanceListSuppliers.toProvider(serviceId, firstInstance), serviceId); - - Response firstResponse = loadBalancer - .choose(new DefaultRequest<>(new RetryableRequestContext(firstInstance))).block(); - - assertThat(firstResponse.getServer()).isEqualTo(firstInstance); - } - -} 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); + } + +} From c17457c8a72e1ec309a548415ab52de56923fd0e Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 29 Sep 2020 19:24:16 +0200 Subject: [PATCH 12/13] Fix property name. --- docs/src/main/asciidoc/_configprops.adoc | 2 +- .../annotation/LoadBalancerClientConfiguration.java | 2 +- .../META-INF/additional-spring-configuration-metadata.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/src/main/asciidoc/_configprops.adoc b/docs/src/main/asciidoc/_configprops.adoc index 89aa68c23..e980076cf 100644 --- a/docs/src/main/asciidoc/_configprops.adoc +++ b/docs/src/main/asciidoc/_configprops.adoc @@ -35,7 +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.avoidPreviousInstance | true | Enables wrapping ServiceInstanceListSupplier beans with `RetryAwareServiceInstanceListSupplier` if Spring-Retry is in the classpath. +|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/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 e5c10ac12..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 @@ -166,7 +166,7 @@ static class LoadBalancerRetryEnabled { } - @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoidPreviousInstance", havingValue = "true", + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoid-previous-instance", havingValue = "true", matchIfMissing = true) static class AvoidPreviousInstanceEnabled { 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 a538cce69..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 @@ -24,7 +24,7 @@ }, { "defaultValue": true, - "name": "spring.cloud.loadbalancer.retry.avoidPreviousInstance", + "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" } From 1d90b7e6ac02eeec007cab2e320ea310aa5fa6c9 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 29 Sep 2020 19:31:39 +0200 Subject: [PATCH 13/13] Change spelling. --- docs/src/main/asciidoc/spring-cloud-commons.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 68a4669a2..d26d4b9ad 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -449,7 +449,7 @@ You can set: - `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 behaviour by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance` to `false`. +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]