Avoid retrying on same instance (#834)

* cherry-pick switching to properties

* Pass information on previous ServiceInstance to RequestContext.

# Conflicts:
#	spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryPolicy.java

* Add a RoundRobinLoadBalancer implementation that avoids same service instance while retrying.

* Wrap instances in ArrayList. Add tests.

* Enable AvoidPreviousInstanceRoundRobinLoadBalancer by default if SpringRetry on classpath.

* Fix failing tests. Add javadocs and author tags.

* Fix properties.

* Add documentation.

* Fix docs after review.

* Fix docs after review.

* Handle avoiding previous instance with ServiceInstanceListSupplier in place of LoadBalancer.

* Fix property name.

* Change spelling.

Author: OlgaMaciaszek

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 <code>hint</code> that is passed on to the LoadBalancer request and can subsequently be used in {@link ReactiveLoadBalancer} implementations.
+|spring.cloud.loadbalancer.retry.avoid-previous-instance | true | Enables wrapping ServiceInstanceListSupplier beans with `RetryAwareServiceInstanceListSupplier` if Spring-Retry is in the classpath.
 |spring.cloud.loadbalancer.retry.enabled | true | 
 |spring.cloud.loadbalancer.retry.max-retries-on-next-service-instance | 1 | Number of retries to be executed on the next <code>ServiceInstance</code>. A <code>ServiceInstance</code> 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 <code>ServiceInstance</code>.

docs/src/main/asciidoc/spring-cloud-commons.adoc

@@ -448,6 +448,9 @@ You can set:
 - `spring.cloud.loadbalancer.retry.maxRetriesOnSameServiceInstance` - indicates how many times a request should be retried on the same `ServiceInstance` (counted separately for every selected instance)
 - `spring.cloud.loadbalancer.retry.maxRetriesOnNextServiceInstance` - indicates how many times a request should be retried a newly selected `ServiceInstance`
 - `spring.cloud.loadbalancer.retry.retryableStatusCodes` - the status codes on which to always retry a failed request.
+
+NOTE: For load-balanced retries, by default, we wrap the `ServiceInstanceListSupplier` bean with `RetryAwareServiceInstanceListSupplier` to select a different instance from the one previously chosen, if available. You can disable this behavior by setting the value of `spring.cloud.loadbalancer.retry.avoidPreviousInstance` to `false`.
+
 ====
 [source,java,indent=0]
 ----

spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/InterceptorRetryPolicy.java

@@ -25,6 +25,7 @@
  * requests.
  *
  * @author Ryan Baxter
+ * @author Olga Maciaszek-Sharma
  */
 public class InterceptorRetryPolicy implements RetryPolicy {
 
@@ -56,7 +57,7 @@ public boolean canRetry(RetryContext context) {
 		LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context;
 		if (lbContext.getRetryCount() == 0 && lbContext.getServiceInstance() == null) {
 			// We haven't even tried to make the request yet so return true so we do
-			lbContext.setServiceInstance(serviceInstanceChooser.choose(serviceName));
+			lbContext.setServiceInstance(null);
 			return true;
 		}
 		return policy.canRetryNextServer(lbContext);

spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancedRetryContext.java

@@ -25,13 +25,16 @@
  * {@link RetryContext} for load-balanced retries.
  *
  * @author Ryan Baxter
+ * @author Olga Maciaszek-Sharma
  */
 public class LoadBalancedRetryContext extends RetryContextSupport {
 
 	private HttpRequest request;
 
 	private ServiceInstance serviceInstance;
 
+	private ServiceInstance previousServiceInstance;
+
 	/**
 	 * Creates a new load-balanced context.
 	 * @param parent The parent context.
@@ -71,7 +74,16 @@ public ServiceInstance getServiceInstance() {
 	 * @param serviceInstance The service instance to use during the retry.
 	 */
 	public void setServiceInstance(ServiceInstance serviceInstance) {
+		setPreviousServiceInstance(this.serviceInstance);
 		this.serviceInstance = serviceInstance;
 	}
 
+	public ServiceInstance getPreviousServiceInstance() {
+		return previousServiceInstance;
+	}
+
+	public void setPreviousServiceInstance(ServiceInstance previousServiceInstance) {
+		this.previousServiceInstance = previousServiceInstance;
+	}
+
 }

spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java

@@ -44,11 +44,12 @@
  * @author Dave Syer
  * @author Will Tran
  * @author Gang Li
+ * @author Olga Maciaszek-Sharma
  */
 @Configuration(proxyBeanMethods = false)
 @ConditionalOnClass(RestTemplate.class)
 @ConditionalOnBean(LoadBalancerClient.class)
-@EnableConfigurationProperties(LoadBalancerRetryProperties.class)
+@EnableConfigurationProperties(LoadBalancerProperties.class)
 public class LoadBalancerAutoConfiguration {
 
 	@LoadBalanced
@@ -125,11 +126,11 @@ public static class RetryInterceptorAutoConfiguration {
 		@Bean
 		@ConditionalOnMissingBean
 		public RetryLoadBalancerInterceptor loadBalancerInterceptor(LoadBalancerClient loadBalancerClient,
-				LoadBalancerRetryProperties retryProperties, LoadBalancerRequestFactory requestFactory,
-				LoadBalancedRetryFactory loadBalancedRetryFactory, LoadBalancerProperties properties,
+				LoadBalancerProperties properties, LoadBalancerRequestFactory requestFactory,
+				LoadBalancedRetryFactory loadBalancedRetryFactory,
 				ReactiveLoadBalancer.Factory<ServiceInstance> loadBalancerFactory) {
-			return new RetryLoadBalancerInterceptor(loadBalancerClient, retryProperties, requestFactory,
-					loadBalancedRetryFactory, properties, loadBalancerFactory);
+			return new RetryLoadBalancerInterceptor(loadBalancerClient, properties, requestFactory,
+					loadBalancedRetryFactory, loadBalancerFactory);
 		}
 
 		@Bean

spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRetryProperties.java

@@ -52,18 +52,15 @@ public class RetryLoadBalancerInterceptor implements ClientHttpRequestIntercepto
 
 	private final LoadBalancedRetryFactory lbRetryFactory;
 
-	private final LoadBalancerRetryProperties retryProperties;
-
 	private final ReactiveLoadBalancer.Factory<ServiceInstance> loadBalancerFactory;
 
-	public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer, LoadBalancerRetryProperties retryProperties,
+	public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer, LoadBalancerProperties properties,
 			LoadBalancerRequestFactory requestFactory, LoadBalancedRetryFactory lbRetryFactory,
-			LoadBalancerProperties properties, ReactiveLoadBalancer.Factory<ServiceInstance> loadBalancerFactory) {
+			ReactiveLoadBalancer.Factory<ServiceInstance> loadBalancerFactory) {
 		this.loadBalancer = loadBalancer;
-		this.retryProperties = retryProperties;
+		this.properties = properties;
 		this.requestFactory = requestFactory;
 		this.lbRetryFactory = lbRetryFactory;
-		this.properties = properties;
 		this.loadBalancerFactory = loadBalancerFactory;
 	}
 
@@ -85,11 +82,21 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body
 					.getSupportedLifecycleProcessors(
 							loadBalancerFactory.getInstances(serviceName, LoadBalancerLifecycle.class),
 							HttpRequestContext.class, ClientHttpResponse.class, ServiceInstance.class);
-			String hint = getHint(serviceName);
-			DefaultRequest<HttpRequestContext> 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<RetryableRequestContext> 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<ServiceInstance> lbResponse = new DefaultResponse(serviceInstance);
 			if (serviceInstance == null) {
@@ -127,7 +134,7 @@ private RetryTemplate createRetryTemplate(String serviceName, HttpRequest reques
 		if (retryListeners != null && retryListeners.length != 0) {
 			template.setListeners(retryListeners);
 		}
-		template.setRetryPolicy(!retryProperties.isEnabled() || retryPolicy == null ? new NeverRetryPolicy()
+		template.setRetryPolicy(!properties.getRetry().isEnabled() || retryPolicy == null ? new NeverRetryPolicy()
 				: new InterceptorRetryPolicy(request, retryPolicy, loadBalancer, serviceName));
 		return template;
 	}

spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright 2012-2020 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.cloud.client.loadbalancer;
+
+import org.springframework.cloud.client.ServiceInstance;
+
+/**
+ * A request context object that allows storing information on previously used service
+ * instances.
+ *
+ * @author Olga Maciaszek-Sharma
+ */
+public class RetryableRequestContext extends DefaultRequestContext {
+
+	private final ServiceInstance previousServiceInstance;
+
+	public RetryableRequestContext(ServiceInstance previousServiceInstance) {
+		this.previousServiceInstance = previousServiceInstance;
+	}
+
+	public RetryableRequestContext(ServiceInstance previousServiceInstance, Object clientRequest) {
+		super(clientRequest);
+		this.previousServiceInstance = previousServiceInstance;
+	}
+
+	public RetryableRequestContext(ServiceInstance previousServiceInstance, Object clientRequest, String hint) {
+		super(clientRequest, hint);
+		this.previousServiceInstance = previousServiceInstance;
+	}
+
+	public ServiceInstance getPreviousServiceInstance() {
+		return previousServiceInstance;
+	}
+
+}