From 342cda80b0b04f6c6d91b0000043f2e74b6e58dc Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 5 Nov 2020 15:33:41 +0100 Subject: [PATCH 01/16] Implement retry logic. --- .../loadbalancer/ClientRequestContext.java | 5 + .../reactive/LoadBalancerProperties.java | 62 ++++ .../reactive/LoadBalancerRetryContext.java | 59 ++++ .../reactive/LoadBalancerRetryPolicy.java | 41 +++ ...torLoadBalancerExchangeFilterFunction.java | 4 +- .../reactive/RetryableErrorException.java | 7 + ...FilterFunctionLoadBalancerRetryPolicy.java | 39 +++ ...bleLoadBalancerExchangeFilterFunction.java | 229 +++++++++++++ .../RetryableStatusCodeException.java | 7 + ...coveryClientBasedReactiveLoadBalancer.java | 47 +++ ...adBalancerExchangeFilterFunctionTests.java | 39 +-- ...adBalancerExchangeFilterFunctionTests.java | 301 ++++++++++++++++++ 12 files changed, 800 insertions(+), 40 deletions(-) create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableErrorException.java create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java create mode 100644 spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java create mode 100644 spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ClientRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ClientRequestContext.java index 4772f351e..8bdbab38f 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ClientRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ClientRequestContext.java @@ -16,6 +16,7 @@ package org.springframework.cloud.client.loadbalancer; +import org.springframework.http.HttpMethod; import org.springframework.web.reactive.function.client.ClientRequest; /** @@ -36,4 +37,8 @@ public ClientRequest getClientRequest() { return (ClientRequest) super.getClientRequest(); } + public HttpMethod method() { + return ((ClientRequest) super.getClientRequest()).method(); + } + } 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 5bbc89df1..475378b6c 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 @@ -141,6 +141,8 @@ public static class Retry { */ private Set retryableStatusCodes = new HashSet<>(); + private Backoff backoff = new Backoff(); + /** * Returns true if the load balancer should retry failed requests. * @return True if the load balancer should retry failed requests; false @@ -190,6 +192,66 @@ public void setRetryableStatusCodes(Set retryableStatusCodes) { this.retryableStatusCodes = retryableStatusCodes; } + public Backoff getBackoff() { + return backoff; + } + + public void setBackoff(Backoff backoff) { + this.backoff = backoff; + } + + public static class Backoff { + + private boolean enabled = false; + + private Duration minBackoff = Duration.ofMillis(5); + + private Duration maxBackoff = Duration.ofMillis(Long.MAX_VALUE); + + private double jitter = 0.5d; + + private boolean basedOnPreviousValue = true; + + public Duration getMinBackoff() { + return minBackoff; + } + + public void setMinBackoff(Duration minBackoff) { + this.minBackoff = minBackoff; + } + + public Duration getMaxBackoff() { + return maxBackoff; + } + + public void setMaxBackoff(Duration maxBackoff) { + this.maxBackoff = maxBackoff; + } + + public double getJitter() { + return jitter; + } + + public void setJitter(double jitter) { + this.jitter = jitter; + } + + public boolean isBasedOnPreviousValue() { + return basedOnPreviousValue; + } + + public void setBasedOnPreviousValue(boolean basedOnPreviousValue) { + this.basedOnPreviousValue = basedOnPreviousValue; + } + + public boolean isEnabled() { + return enabled; + } + + public void setEnabled(boolean enabled) { + this.enabled = enabled; + } + } } } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java new file mode 100644 index 000000000..39758d5c3 --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java @@ -0,0 +1,59 @@ +package org.springframework.cloud.client.loadbalancer.reactive; + +import org.springframework.http.HttpMethod; +import org.springframework.web.reactive.function.client.ClientRequest; +import org.springframework.web.reactive.function.client.ClientResponse; + +/** + * @author Olga Maciaszek-Sharma + */ +public class LoadBalancerRetryContext { + private final ClientRequest request; + private ClientResponse clientResponse; + private Integer retriesSameServiceInstance = 0; + private Integer retriesNextServiceInstance = 0; + + LoadBalancerRetryContext(ClientRequest request) { + this.request = request; + } + + ClientRequest getRequest() { + return request; + } + + ClientResponse getClientResponse() { + return clientResponse; + } + + void setClientResponse(ClientResponse clientResponse) { + this.clientResponse = clientResponse; + } + + Integer getRetriesSameServiceInstance() { + return retriesSameServiceInstance; + } + + void incrementRetriesSameServiceInstance() { + retriesSameServiceInstance++; + } + + void resetRetriesSameServiceInstance() { + retriesSameServiceInstance = 0; + } + + Integer getRetriesNextServiceInstance() { + return retriesNextServiceInstance; + } + + void incrementRetriesNextServiceInstance() { + retriesNextServiceInstance++; + } + + Integer getResponseStatusCode() { + return clientResponse.statusCode().value(); + } + + HttpMethod getRequestMethod() { + return request.method(); + } +} \ No newline at end of file diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java new file mode 100644 index 000000000..268de330e --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java @@ -0,0 +1,41 @@ +package org.springframework.cloud.client.loadbalancer.reactive; + +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; + +/** + * @author Olga Maciaszek-Sharma + */ +public interface LoadBalancerRetryPolicy { + + /** + * Return true to retry the failed request on the same server. This method may be + * called more than once when executing a single operation. + * @param context The context for the retry operation. + * @return True to retry the failed request on the same server; false otherwise. + */ + boolean canRetrySameServiceInstance(LoadBalancerRetryContext context); + + /** + * Return true to retry the failed request on the next server from the load balancer. + * This method may be called more than once when executing a single operation. + * @param context The context for the retry operation. + * @return True to retry the failed request on the next server from the load balancer; + * false otherwise. + */ + boolean canRetryNextServiceInstance(LoadBalancerRetryContext context); + + /** + * If an exception is not thrown when making a request, this method will be called to + * see if the client would like to retry the request based on the status code + * returned. For example, in Cloud Foundry, the router will return a 404 + * when an app is not available. Since HTTP clients do not throw an exception when a + * 404 is returned, retryableStatusCode allows clients to + * force a retry. + * @param statusCode The HTTP status code. + * @return True if a retry should be attempted; false to just return the response. + */ + boolean retryableStatusCode(int statusCode); + + boolean canRetryOnMethod(HttpMethod method); +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index 0615bed59..18b81b4d5 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -94,7 +94,7 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction } if (LOG.isDebugEnabled()) { - LOG.debug(String.format("Load balancer has retrieved the instance for service %s: %s", serviceId, + LOG.debug(String.format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, instance.getUri())); } ClientRequest newRequest = buildClientRequest(clientRequest, reconstructURI(instance, originalUrl)); @@ -121,7 +121,7 @@ protected Mono> choose(String serviceId, Request> exceptions = Arrays + .asList(IOException.class, TimeoutException.class, + org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); + private final LoadBalancerRetryPolicy retryPolicy; + private final LoadBalancerProperties properties; + private final ReactiveLoadBalancer.Factory loadBalancerFactory; + + // TODO: move backoff config up to Policy? // user factory and create policy here instead? + public RetryableLoadBalancerExchangeFilterFunction(LoadBalancerRetryPolicy retryPolicy, + LoadBalancerProperties properties, ReactiveLoadBalancer.Factory loadBalancerFactory) { + this.retryPolicy = retryPolicy; + this.properties = properties; + this.loadBalancerFactory = loadBalancerFactory; + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + @Override + public Mono filter(ClientRequest clientRequest, ExchangeFunction next) { + LoadBalancerRetryContext loadBalancerRetryContext = new LoadBalancerRetryContext(clientRequest); + Retry exchangeRetry = buildRetrySpec(properties.getRetry() + .getMaxRetriesOnSameServiceInstance(), true); + Retry filterRetry = buildRetrySpec(properties.getRetry() + .getMaxRetriesOnNextServiceInstance(), false); + + URI originalUrl = clientRequest.url(); + String serviceId = originalUrl.getHost(); + if (serviceId == null) { + String message = String + .format("Request URI does not contain a valid hostname: %s", originalUrl + .toString()); + if (LOG.isWarnEnabled()) { + LOG.warn(message); + } + return Mono.just(ClientResponse.create(HttpStatus.BAD_REQUEST).body(message) + .build()); + } + Set supportedLifecycleProcessors = LoadBalancerLifecycleValidator + .getSupportedLifecycleProcessors( + loadBalancerFactory + .getInstances(serviceId, LoadBalancerLifecycle.class), + ClientRequestContext.class, ClientResponse.class, ServiceInstance.class); + String hint = getHint(serviceId); + DefaultRequest lbRequest = new DefaultRequest<>( + new RetryableRequestContext(null, clientRequest, hint)); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); + return Mono.defer(() -> choose(serviceId, lbRequest).flatMap(lbResponse -> { + ServiceInstance instance = lbResponse.getServer(); + lbRequest + .setContext(new RetryableRequestContext(instance, clientRequest, hint)); + if (instance == null) { + String message = serviceInstanceUnavailableMessage(serviceId); + if (LOG.isWarnEnabled()) { + LOG.warn(message); + } + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse))); + return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) + .body(serviceInstanceUnavailableMessage(serviceId)).build()); + } + + if (LOG.isDebugEnabled()) { + LOG.debug(String + .format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, + instance.getUri())); + } + ClientRequest newRequest = buildClientRequest(clientRequest, reconstructURI(instance, originalUrl)); + return next.exchange(newRequest) + .doOnError(throwable -> supportedLifecycleProcessors.forEach( + lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.FAILED, throwable, lbResponse)))) + .doOnSuccess(clientResponse -> + supportedLifecycleProcessors.forEach( + lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.SUCCESS, lbResponse, clientResponse)))) + .map(clientResponse -> { + loadBalancerRetryContext.setClientResponse(clientResponse); + if (shouldRetrySameServiceInstance(loadBalancerRetryContext)) { + if (LOG.isDebugEnabled()) { + LOG.debug(String + .format("Retrying on status code: %d", clientResponse + .statusCode().value())); + } + throw new RetryableStatusCodeException(); + } + return clientResponse; + + }); + }) + .map(clientResponse -> { + loadBalancerRetryContext.setClientResponse(clientResponse); + if (shouldRetryNextServiceInstance(loadBalancerRetryContext)) { + if (LOG.isDebugEnabled()) { + LOG.debug(String + .format("Retrying on status code: %d", clientResponse + .statusCode().value())); + } + throw new RetryableStatusCodeException(); + } + return clientResponse; + + } + ).onErrorMap(throwable -> new RetryableErrorException()) + .retryWhen(exchangeRetry) + ) + .retryWhen(filterRetry); + } + + @NotNull + private Retry buildRetrySpec(int max, boolean transientErrors) { + LoadBalancerProperties.Retry.Backoff backoffProperties = properties.getRetry() + .getBackoff(); + if (backoffProperties.isEnabled()) { + return RetrySpec + .backoff(max, backoffProperties.getMinBackoff()) + .filter(this::isRetryException) + .maxBackoff(backoffProperties.getMaxBackoff()) + .jitter(backoffProperties.getJitter()) + .transientErrors(transientErrors); + } + return RetrySpec + .max(max) + .filter(this::isRetryException) + .transientErrors(transientErrors); + } + + private boolean shouldRetrySameServiceInstance(LoadBalancerRetryContext loadBalancerRetryContext) { + boolean shouldRetry = retryPolicy + .retryableStatusCode(loadBalancerRetryContext.getResponseStatusCode()) + && retryPolicy + .canRetryOnMethod(loadBalancerRetryContext.getRequestMethod()) + && retryPolicy.canRetrySameServiceInstance(loadBalancerRetryContext); + if (shouldRetry) { + loadBalancerRetryContext.incrementRetriesSameServiceInstance(); + } + return shouldRetry; + } + + private boolean shouldRetryNextServiceInstance(LoadBalancerRetryContext loadBalancerRetryContext) { + boolean shouldRetry = retryPolicy + .retryableStatusCode(loadBalancerRetryContext.getResponseStatusCode()) && + retryPolicy.canRetryOnMethod(loadBalancerRetryContext.getRequestMethod()) + && retryPolicy.canRetryNextServiceInstance(loadBalancerRetryContext); + if (shouldRetry) { + loadBalancerRetryContext.incrementRetriesNextServiceInstance(); + loadBalancerRetryContext.resetRetriesSameServiceInstance(); + } + return shouldRetry; + } + + private boolean isRetryException(Throwable throwable) { + return exceptions.stream() + .anyMatch(exception -> exception + .isInstance(throwable) || throwable != null && exception + .isInstance(throwable.getCause())); + } + + protected URI reconstructURI(ServiceInstance instance, URI original) { + return LoadBalancerUriTools.reconstructURI(instance, original); + } + + protected Mono> choose(String serviceId, Request request) { + ReactiveLoadBalancer loadBalancer = loadBalancerFactory + .getInstance(serviceId); + if (loadBalancer == null) { + return Mono.just(new EmptyResponse()); + } + return Mono.from(loadBalancer.choose(request)); + } + + private String serviceInstanceUnavailableMessage(String serviceId) { + return "Load balancer does not contain an instance for the service " + serviceId; + } + + private ClientRequest buildClientRequest(ClientRequest request, URI uri) { + return ClientRequest.create(request.method(), uri) + .headers(headers -> headers.addAll(request.headers())) + .cookies(cookies -> cookies.addAll(request.cookies())) + .attributes(attributes -> attributes.putAll(request.attributes())) + .body(request.body()).build(); + } + + private String getHint(String serviceId) { + String defaultHint = properties.getHint().getOrDefault("default", "default"); + String hintPropertyValue = properties.getHint().get(serviceId); + return hintPropertyValue != null ? hintPropertyValue : defaultHint; + } +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java new file mode 100644 index 000000000..8ad5541a5 --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java @@ -0,0 +1,7 @@ +package org.springframework.cloud.client.loadbalancer.reactive; + +/** + * @author Olga Maciaszek-Sharma + */ +public class RetryableStatusCodeException extends IllegalStateException { +} diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java new file mode 100644 index 000000000..a4579acbc --- /dev/null +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java @@ -0,0 +1,47 @@ +package org.springframework.cloud.client.loadbalancer.reactive; + +import java.util.List; +import java.util.Random; + +import org.reactivestreams.Publisher; +import reactor.core.publisher.Mono; + +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.discovery.DiscoveryClient; +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; + +/** + * @author Olga Maciaszek-Sharma + */ +class DiscoveryClientBasedReactiveLoadBalancer implements ReactiveLoadBalancer { + + private final Random random = new Random(); + + private final String serviceId; + + private final DiscoveryClient discoveryClient; + + DiscoveryClientBasedReactiveLoadBalancer(String serviceId, DiscoveryClient discoveryClient) { + this.serviceId = serviceId; + this.discoveryClient = discoveryClient; + } + + @Override + public Publisher> choose() { + List instances = discoveryClient.getInstances(serviceId); + if (instances.size() == 0) { + return Mono.just(new EmptyResponse()); + } + int instanceIdx = this.random.nextInt(instances.size()); + return Mono.just(new DefaultResponse(instances.get(instanceIdx))); + } + + @Override + public Publisher> choose(Request request) { + return choose(); + } + +} \ No newline at end of file diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java index cf76b5d36..830bdede8 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java @@ -20,17 +20,13 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Random; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; -import org.reactivestreams.Publisher; -import reactor.core.publisher.Mono; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.SpringBootConfiguration; @@ -44,11 +40,8 @@ import org.springframework.cloud.client.discovery.simple.SimpleDiscoveryProperties; import org.springframework.cloud.client.loadbalancer.CompletionContext; import org.springframework.cloud.client.loadbalancer.DefaultRequestContext; -import org.springframework.cloud.client.loadbalancer.DefaultResponse; -import org.springframework.cloud.client.loadbalancer.EmptyResponse; import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; import org.springframework.cloud.client.loadbalancer.Request; -import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.context.annotation.Bean; import org.springframework.http.HttpStatus; import org.springframework.web.bind.annotation.RequestMapping; @@ -246,34 +239,4 @@ protected String getName() { } -} - -class DiscoveryClientBasedReactiveLoadBalancer implements ReactiveLoadBalancer { - - private final Random random = new Random(); - - private final String serviceId; - - private final DiscoveryClient discoveryClient; - - DiscoveryClientBasedReactiveLoadBalancer(String serviceId, DiscoveryClient discoveryClient) { - this.serviceId = serviceId; - this.discoveryClient = discoveryClient; - } - - @Override - public Publisher> choose() { - List instances = discoveryClient.getInstances(serviceId); - if (instances.size() == 0) { - return Mono.just(new EmptyResponse()); - } - int instanceIdx = this.random.nextInt(instances.size()); - return Mono.just(new DefaultResponse(instances.get(instanceIdx))); - } - - @Override - public Publisher> choose(Request request) { - return choose(); - } - -} +} \ No newline at end of file diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java new file mode 100644 index 000000000..e3dc7d006 --- /dev/null +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -0,0 +1,301 @@ +package org.springframework.cloud.client.loadbalancer.reactive; + +import java.net.URI; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.web.server.LocalServerPort; +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.client.discovery.EnableDiscoveryClient; +import org.springframework.cloud.client.discovery.simple.SimpleDiscoveryProperties; +import org.springframework.cloud.client.loadbalancer.CompletionContext; +import org.springframework.cloud.client.loadbalancer.DefaultRequestContext; +import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.context.annotation.Bean; +import org.springframework.http.HttpStatus; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.reactive.function.client.ClientResponse; +import org.springframework.web.reactive.function.client.WebClient; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.BDDAssertions.then; +import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; + +/** + * @author Olga Maciaszek-Sharma + */ +@SpringBootTest(webEnvironment = RANDOM_PORT) +public class RetryableLoadBalancerExchangeFilterFunctionTests { + + @Autowired + private RetryableLoadBalancerExchangeFilterFunction loadBalancerFunction; + + @Autowired + private SimpleDiscoveryProperties properties; + + @Autowired + private LoadBalancerProperties loadBalancerProperties; + + @Autowired + private ReactiveLoadBalancer.Factory factory; + + @LocalServerPort + private int port; + + @BeforeEach + void setUp() { + DefaultServiceInstance instance = new DefaultServiceInstance(); + instance.setServiceId("testservice"); + instance.setUri(URI.create("http://localhost:" + port)); + DefaultServiceInstance instanceWithNoLifecycleProcessors = new DefaultServiceInstance(); + instanceWithNoLifecycleProcessors + .setServiceId("serviceWithNoLifecycleProcessors"); + instanceWithNoLifecycleProcessors + .setUri(URI.create("http://localhost:" + port)); + properties.getInstances().put("testservice", Collections.singletonList(instance)); + properties.getInstances().put("serviceWithNoLifecycleProcessors", + Collections.singletonList(instanceWithNoLifecycleProcessors)); + } + + @Test + void correctResponseReturnedForExistingHostAndInstancePresent() { + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/hello") + .exchange() + .block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + } + + @Test + void correctResponseReturnedAfterRetryingOnSameServiceInstance() { + loadBalancerProperties.getRetry().setMaxRetriesOnSameServiceInstance(1); + loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/exception") + .exchange() + .block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World!"); + } + + @Test + void correctResponseReturnedAfterRetryingOnNextServiceInstance() { + loadBalancerProperties.getRetry().setMaxRetriesOnSameServiceInstance(1); + DefaultServiceInstance goodRetryTestInstance = new DefaultServiceInstance(); + goodRetryTestInstance.setServiceId("retrytest"); + goodRetryTestInstance.setUri(URI.create("http://localhost:" + port)); + DefaultServiceInstance badRetryTestInstance = new DefaultServiceInstance(); + badRetryTestInstance.setServiceId("retrytest"); + badRetryTestInstance.setUri(URI.create("http://localhost:" + 8080)); + properties.getInstances().put("retrytest", + Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); + loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://retrytest") + .filter(this.loadBalancerFunction).build().get().uri("/hello") + .exchange() + .block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + } + + + @Test + void serviceUnavailableReturnedWhenNoInstancePresent() { + ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx") + .filter(this.loadBalancerFunction) + .build().get().exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); + } + + @Test + @Disabled + // FIXME 3.0.0 + void badRequestReturnedForIncorrectHost() { + ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx") + .filter(this.loadBalancerFunction) + .build().get().exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + } + + @Test + void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { + assertThatCode(() -> WebClient.builder() + .baseUrl("http://serviceWithNoLifecycleProcessors") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() + .block()) + .doesNotThrowAnyException(); + } + + @Test + void loadBalancerLifecycleCallbacksExecuted() { + final String callbackTestHint = "callbackTestHint"; + loadBalancerProperties.getHint().put("testservice", "callbackTestHint"); + final String result = "callbackTestResult"; + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/callback") + .exchange().block(); + + Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class) + .get("loadBalancerLifecycle")).getStartLog() + .values(); + Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class) + .get("anotherLoadBalancerLifecycle")) + .getCompleteLog().values(); + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + assertThat(lifecycleLogRequests) + .extracting(request -> ((DefaultRequestContext) request.getContext()) + .getHint()) + .contains(callbackTestHint); + assertThat(anotherLifecycleLogRequests) + .extracting(completionContext -> ((ClientResponse) completionContext + .getClientResponse()) + .bodyToMono(String.class).block()) + .contains(result); + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + @EnableDiscoveryClient + @EnableAutoConfiguration + @SpringBootConfiguration(proxyBeanMethods = false) + @RestController + static class Config { + + AtomicInteger exceptionCallsCount = new AtomicInteger(); + + @GetMapping("/hello") + public String hello() { + return "Hello World"; + } + + @GetMapping("/callback") + String callbackTestResult() { + return "callbackTestResult"; + } + + @GetMapping("/exception") + String exception() { + int callCount = exceptionCallsCount.incrementAndGet(); + if (callCount % 2 != 0) { + throw new IllegalStateException("Test!"); + } + return "Hello World!"; + } + + @Bean + ReactiveLoadBalancer.Factory reactiveLoadBalancerFactory(DiscoveryClient discoveryClient) { + return new ReactiveLoadBalancer.Factory() { + + private final TestLoadBalancerLifecycle testLoadBalancerLifecycle = new TestLoadBalancerLifecycle(); + + private final TestLoadBalancerLifecycle anotherLoadBalancerLifecycle = new AnotherLoadBalancerLifecycle(); + + @Override + public ReactiveLoadBalancer getInstance(String serviceId) { + return new org.springframework.cloud.client.loadbalancer.reactive.DiscoveryClientBasedReactiveLoadBalancer(serviceId, discoveryClient); + } + + @Override + public Map getInstances(String name, Class type) { + if (name.equals("serviceWithNoLifecycleProcessors")) { + return null; + } + Map lifecycleProcessors = new HashMap<>(); + lifecycleProcessors + .put("loadBalancerLifecycle", testLoadBalancerLifecycle); + lifecycleProcessors + .put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); + return lifecycleProcessors; + } + + @Override + public X getInstance(String name, Class clazz, Class... generics) { + return null; + } + }; + } + + @Bean + LoadBalancerProperties loadBalancerProperties() { + return new LoadBalancerProperties(); + } + + @Bean + RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerProperties properties, + ReactiveLoadBalancer.Factory factory) { + return new RetryableLoadBalancerExchangeFilterFunction(new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), + properties, factory); + } + } + + protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { + + ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); + + ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + + @Override + public void onStart(Request request) { + startLog.put(getName() + UUID.randomUUID(), request); + } + + @Override + public void onComplete(CompletionContext completionContext) { + completeLog.put(getName() + UUID.randomUUID(), completionContext); + } + + ConcurrentHashMap> getStartLog() { + return startLog; + } + + ConcurrentHashMap> getCompleteLog() { + return completeLog; + } + + protected String getName() { + return this.getClass().getSimpleName(); + } + + } + + protected static class AnotherLoadBalancerLifecycle extends TestLoadBalancerLifecycle { + + @Override + protected String getName() { + return this.getClass().getSimpleName(); + } + + } + +} + + From b0f476284ee05f42f1be8e6606985711dbc244cc Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 5 Nov 2020 17:13:47 +0100 Subject: [PATCH 02/16] Fix retrying on next instance when RetryExhausted in same instance. --- ...bleLoadBalancerExchangeFilterFunction.java | 7 ++++--- ...coveryClientBasedReactiveLoadBalancer.java | 21 ++++++++++++++++++- ...adBalancerExchangeFilterFunctionTests.java | 2 +- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index 9bad62f73..f78f6ecc2 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -45,7 +45,6 @@ public class RetryableLoadBalancerExchangeFilterFunction implements ExchangeFilt private final LoadBalancerProperties properties; private final ReactiveLoadBalancer.Factory loadBalancerFactory; - // TODO: move backoff config up to Policy? // user factory and create policy here instead? public RetryableLoadBalancerExchangeFilterFunction(LoadBalancerRetryPolicy retryPolicy, LoadBalancerProperties properties, ReactiveLoadBalancer.Factory loadBalancerFactory) { this.retryPolicy = retryPolicy; @@ -141,7 +140,7 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction return clientResponse; } - ).onErrorMap(throwable -> new RetryableErrorException()) + ) .retryWhen(exchangeRetry) ) .retryWhen(filterRetry); @@ -193,7 +192,9 @@ private boolean isRetryException(Throwable throwable) { return exceptions.stream() .anyMatch(exception -> exception .isInstance(throwable) || throwable != null && exception - .isInstance(throwable.getCause())); + .isInstance(throwable.getCause()) + || throwable.getClass().getName() + .contains("RetryExhaustedException")); } protected URI reconstructURI(ServiceInstance instance, URI original) { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java index a4579acbc..74d07209c 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java @@ -12,6 +12,7 @@ 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.RetryableRequestContext; /** * @author Olga Maciaszek-Sharma @@ -41,7 +42,25 @@ public Publisher> choose() { @Override public Publisher> choose(Request request) { - return choose(); + + List instances = discoveryClient.getInstances(serviceId); + if (request.getContext() instanceof RetryableRequestContext) { + RetryableRequestContext context = (RetryableRequestContext) request + .getContext(); + if (context.getPreviousServiceInstance() != null) { + List instancesCopy = discoveryClient + .getInstances(serviceId); + instancesCopy.remove(context.getPreviousServiceInstance()); + if (!instancesCopy.isEmpty()) { + instances = instancesCopy; + } + } + } + if (instances.size() == 0) { + return Mono.just(new EmptyResponse()); + } + int instanceIdx = this.random.nextInt(instances.size()); + return Mono.just(new DefaultResponse(instances.get(instanceIdx))); } } \ No newline at end of file diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index e3dc7d006..3163fe3a6 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -244,6 +244,7 @@ public X getInstance(String name, Class clazz, Class... generics) { }; } + @Bean LoadBalancerProperties loadBalancerProperties() { return new LoadBalancerProperties(); @@ -295,7 +296,6 @@ protected String getName() { } } - } From 85145dd9c6667ff8a118d18662a71d5f804736c3 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 5 Nov 2020 17:18:53 +0100 Subject: [PATCH 03/16] Fix retrying on next instance when RetryExhausted in same instance. --- .../RetryableLoadBalancerExchangeFilterFunction.java | 2 +- .../RetryableLoadBalancerExchangeFilterFunctionTests.java | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index f78f6ecc2..a1a03a3ec 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -194,7 +194,7 @@ private boolean isRetryException(Throwable throwable) { .isInstance(throwable) || throwable != null && exception .isInstance(throwable.getCause()) || throwable.getClass().getName() - .contains("RetryExhaustedException")); + .equals("reactor.core.Exceptions$RetryExhaustedException")); } protected URI reconstructURI(ServiceInstance instance, URI original) { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index 3163fe3a6..d7c5f299b 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -121,6 +121,14 @@ void correctResponseReturnedAfterRetryingOnNextServiceInstance() { then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + + ClientResponse secondClientResponse = WebClient.builder().baseUrl("http://retrytest") + .filter(this.loadBalancerFunction).build().get().uri("/hello") + .exchange() + .block(); + + then(secondClientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(secondClientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); } From 2ec6268d8e9125cc30256af8cc7adc2bf80affad Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 5 Nov 2020 17:26:44 +0100 Subject: [PATCH 04/16] Fix retrying on next instance when RetryExhausted in same instance. --- .../reactive/RetryableLoadBalancerExchangeFilterFunction.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index a1a03a3ec..c3b7c8da8 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -10,6 +10,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.jetbrains.annotations.NotNull; +import reactor.core.Exceptions; import reactor.core.publisher.Mono; import reactor.util.retry.Retry; import reactor.util.retry.RetrySpec; @@ -193,8 +194,7 @@ private boolean isRetryException(Throwable throwable) { .anyMatch(exception -> exception .isInstance(throwable) || throwable != null && exception .isInstance(throwable.getCause()) - || throwable.getClass().getName() - .equals("reactor.core.Exceptions$RetryExhaustedException")); + || Exceptions.isRetryExhausted(throwable)); } protected URI reconstructURI(ServiceInstance instance, URI original) { From 6536b6dbea781c3f877636a0d1f42cb39ea48876 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 5 Nov 2020 19:31:40 +0100 Subject: [PATCH 05/16] Move duplicated methods to utility class. Fix checkstyle. --- .../reactive/ExchangeFilterFunctionTools.java | 49 ++++ .../reactive/LoadBalancerProperties.java | 2 + .../reactive/LoadBalancerRetryContext.java | 23 +- .../reactive/LoadBalancerRetryPolicy.java | 18 +- ...torLoadBalancerExchangeFilterFunction.java | 28 +-- .../reactive/RetryableErrorException.java | 7 - ...FilterFunctionLoadBalancerRetryPolicy.java | 27 ++- ...bleLoadBalancerExchangeFilterFunction.java | 217 ++++++++---------- .../RetryableStatusCodeException.java | 17 ++ ...coveryClientBasedReactiveLoadBalancer.java | 24 +- ...adBalancerExchangeFilterFunctionTests.java | 2 +- ...adBalancerExchangeFilterFunctionTests.java | 130 +++++------ 12 files changed, 309 insertions(+), 235 deletions(-) create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java delete mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableErrorException.java diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java new file mode 100644 index 000000000..a94b9cf74 --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.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.reactive; + +import java.net.URI; +import java.util.Map; + +import org.springframework.web.reactive.function.client.ClientRequest; + +/** + * @author Olga Maciaszek-Sharma + */ +public final class ExchangeFilterFunctionTools { + + private ExchangeFilterFunctionTools() { + throw new IllegalStateException("Can't instantiate a utility class."); + } + + static String getHint(String serviceId, Map hints) { + String defaultHint = hints.getOrDefault("default", "default"); + String hintPropertyValue = hints.get(serviceId); + return hintPropertyValue != null ? hintPropertyValue : defaultHint; + } + + static ClientRequest buildClientRequest(ClientRequest request, URI uri) { + return ClientRequest.create(request.method(), uri).headers(headers -> headers.addAll(request.headers())) + .cookies(cookies -> cookies.addAll(request.cookies())) + .attributes(attributes -> attributes.putAll(request.attributes())).body(request.body()).build(); + } + + static String serviceInstanceUnavailableMessage(String serviceId) { + return "LoadBalancer does not contain an instance for the service " + serviceId; + } + +} 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 475378b6c..7aae1799a 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 @@ -251,7 +251,9 @@ public boolean isEnabled() { public void setEnabled(boolean enabled) { this.enabled = enabled; } + } + } } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java index 39758d5c3..29a1b8d12 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.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.client.loadbalancer.reactive; import org.springframework.http.HttpMethod; @@ -8,9 +24,13 @@ * @author Olga Maciaszek-Sharma */ public class LoadBalancerRetryContext { + private final ClientRequest request; + private ClientResponse clientResponse; + private Integer retriesSameServiceInstance = 0; + private Integer retriesNextServiceInstance = 0; LoadBalancerRetryContext(ClientRequest request) { @@ -56,4 +76,5 @@ Integer getResponseStatusCode() { HttpMethod getRequestMethod() { return request.method(); } -} \ No newline at end of file + +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java index 268de330e..89888b739 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java @@ -1,7 +1,22 @@ +/* + * 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.reactive; import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatus; /** * @author Olga Maciaszek-Sharma @@ -38,4 +53,5 @@ public interface LoadBalancerRetryPolicy { boolean retryableStatusCode(int statusCode); boolean canRetryOnMethod(HttpMethod method); + } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index 18b81b4d5..f8a7948a4 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -30,7 +30,6 @@ import org.springframework.cloud.client.loadbalancer.EmptyResponse; import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycleValidator; -import org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools; import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.http.HttpStatus; @@ -39,6 +38,11 @@ import org.springframework.web.reactive.function.client.ExchangeFilterFunction; import org.springframework.web.reactive.function.client.ExchangeFunction; +import static org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools.reconstructURI; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.buildClientRequest; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.getHint; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.serviceInstanceUnavailableMessage; + /** * An {@link ExchangeFilterFunction} that uses {@link ReactiveLoadBalancer} to execute * requests against a correct {@link ServiceInstance}. @@ -76,7 +80,7 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction .getSupportedLifecycleProcessors( loadBalancerFactory.getInstances(serviceId, LoadBalancerLifecycle.class), ClientRequestContext.class, ClientResponse.class, ServiceInstance.class); - String hint = getHint(serviceId); + String hint = getHint(serviceId, properties.getHint()); DefaultRequest lbRequest = new DefaultRequest<>( new ClientRequestContext(clientRequest, hint)); supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); @@ -108,10 +112,6 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction }); } - protected URI reconstructURI(ServiceInstance instance, URI original) { - return LoadBalancerUriTools.reconstructURI(instance, original); - } - protected Mono> choose(String serviceId, Request request) { ReactiveLoadBalancer loadBalancer = loadBalancerFactory.getInstance(serviceId); if (loadBalancer == null) { @@ -120,20 +120,4 @@ protected Mono> choose(String serviceId, Request headers.addAll(request.headers())) - .cookies(cookies -> cookies.addAll(request.cookies())) - .attributes(attributes -> attributes.putAll(request.attributes())).body(request.body()).build(); - } - - private String getHint(String serviceId) { - String defaultHint = properties.getHint().getOrDefault("default", "default"); - String hintPropertyValue = properties.getHint().get(serviceId); - return hintPropertyValue != null ? hintPropertyValue : defaultHint; - } - } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableErrorException.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableErrorException.java deleted file mode 100644 index 41509b2e2..000000000 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableErrorException.java +++ /dev/null @@ -1,7 +0,0 @@ -package org.springframework.cloud.client.loadbalancer.reactive; - -/** - * @author Olga Maciaszek-Sharma - */ -public class RetryableErrorException extends IllegalStateException { -} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java index 1fb6d5aa1..bca6c571e 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.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.client.loadbalancer.reactive; import org.springframework.http.HttpMethod; @@ -7,7 +23,6 @@ */ public class RetryableExchangeFilterFunctionLoadBalancerRetryPolicy implements LoadBalancerRetryPolicy { - private final LoadBalancerProperties properties; public RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(LoadBalancerProperties properties) { @@ -16,14 +31,12 @@ public RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(LoadBalancerProper @Override public boolean canRetrySameServiceInstance(LoadBalancerRetryContext context) { - return context.getRetriesSameServiceInstance() < properties.getRetry() - .getMaxRetriesOnSameServiceInstance(); + return context.getRetriesSameServiceInstance() < properties.getRetry().getMaxRetriesOnSameServiceInstance(); } @Override public boolean canRetryNextServiceInstance(LoadBalancerRetryContext context) { - return context.getRetriesNextServiceInstance() < properties.getRetry() - .getMaxRetriesOnNextServiceInstance(); + return context.getRetriesNextServiceInstance() < properties.getRetry().getMaxRetriesOnNextServiceInstance(); } @Override @@ -33,7 +46,7 @@ public boolean retryableStatusCode(int statusCode) { @Override public boolean canRetryOnMethod(HttpMethod method) { - return HttpMethod.GET.equals(method) || properties.getRetry() - .isRetryOnAllOperations(); + return HttpMethod.GET.equals(method) || properties.getRetry().isRetryOnAllOperations(); } + } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index c3b7c8da8..6ad22a1de 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.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.client.loadbalancer.reactive; import java.io.IOException; @@ -22,7 +38,6 @@ import org.springframework.cloud.client.loadbalancer.EmptyResponse; import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycleValidator; -import org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools; import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; @@ -32,18 +47,26 @@ import org.springframework.web.reactive.function.client.ExchangeFilterFunction; import org.springframework.web.reactive.function.client.ExchangeFunction; +import static org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools.reconstructURI; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.buildClientRequest; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.getHint; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.serviceInstanceUnavailableMessage; + /** * @author Olga Maciaszek-Sharma */ public class RetryableLoadBalancerExchangeFilterFunction implements ExchangeFilterFunction { - private static final Log LOG = LogFactory - .getLog(RetryableLoadBalancerExchangeFilterFunction.class); - private static final List> exceptions = Arrays - .asList(IOException.class, TimeoutException.class, - org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); + private static final Log LOG = LogFactory.getLog(RetryableLoadBalancerExchangeFilterFunction.class); + + private static final List> exceptions = Arrays.asList(IOException.class, + TimeoutException.class, + org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); + private final LoadBalancerRetryPolicy retryPolicy; + private final LoadBalancerProperties properties; + private final ReactiveLoadBalancer.Factory loadBalancerFactory; public RetryableLoadBalancerExchangeFilterFunction(LoadBalancerRetryPolicy retryPolicy, @@ -53,123 +76,95 @@ public RetryableLoadBalancerExchangeFilterFunction(LoadBalancerRetryPolicy retry this.loadBalancerFactory = loadBalancerFactory; } - @SuppressWarnings({"rawtypes", "unchecked"}) + @SuppressWarnings({ "rawtypes", "unchecked" }) @Override public Mono filter(ClientRequest clientRequest, ExchangeFunction next) { LoadBalancerRetryContext loadBalancerRetryContext = new LoadBalancerRetryContext(clientRequest); - Retry exchangeRetry = buildRetrySpec(properties.getRetry() - .getMaxRetriesOnSameServiceInstance(), true); - Retry filterRetry = buildRetrySpec(properties.getRetry() - .getMaxRetriesOnNextServiceInstance(), false); + Retry exchangeRetry = buildRetrySpec(properties.getRetry().getMaxRetriesOnSameServiceInstance(), true); + Retry filterRetry = buildRetrySpec(properties.getRetry().getMaxRetriesOnNextServiceInstance(), false); URI originalUrl = clientRequest.url(); String serviceId = originalUrl.getHost(); if (serviceId == null) { - String message = String - .format("Request URI does not contain a valid hostname: %s", originalUrl - .toString()); + String message = String.format("Request URI does not contain a valid hostname: %s", originalUrl.toString()); if (LOG.isWarnEnabled()) { LOG.warn(message); } - return Mono.just(ClientResponse.create(HttpStatus.BAD_REQUEST).body(message) - .build()); + return Mono.just(ClientResponse.create(HttpStatus.BAD_REQUEST).body(message).build()); } Set supportedLifecycleProcessors = LoadBalancerLifecycleValidator .getSupportedLifecycleProcessors( - loadBalancerFactory - .getInstances(serviceId, LoadBalancerLifecycle.class), + loadBalancerFactory.getInstances(serviceId, LoadBalancerLifecycle.class), ClientRequestContext.class, ClientResponse.class, ServiceInstance.class); - String hint = getHint(serviceId); + String hint = getHint(serviceId, properties.getHint()); DefaultRequest lbRequest = new DefaultRequest<>( new RetryableRequestContext(null, clientRequest, hint)); supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); return Mono.defer(() -> choose(serviceId, lbRequest).flatMap(lbResponse -> { - ServiceInstance instance = lbResponse.getServer(); - lbRequest - .setContext(new RetryableRequestContext(instance, clientRequest, hint)); - if (instance == null) { - String message = serviceInstanceUnavailableMessage(serviceId); - if (LOG.isWarnEnabled()) { - LOG.warn(message); + ServiceInstance instance = lbResponse.getServer(); + lbRequest.setContext(new RetryableRequestContext(instance, clientRequest, hint)); + if (instance == null) { + String message = serviceInstanceUnavailableMessage(serviceId); + if (LOG.isWarnEnabled()) { + LOG.warn(message); + } + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse))); + return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) + .body(serviceInstanceUnavailableMessage(serviceId)).build()); + } + + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, + instance.getUri())); + } + ClientRequest newRequest = buildClientRequest(clientRequest, reconstructURI(instance, originalUrl)); + return next.exchange(newRequest) + .doOnError(throwable -> supportedLifecycleProcessors.forEach( + lifecycle -> lifecycle.onComplete(new CompletionContext( + CompletionContext.Status.FAILED, throwable, lbResponse)))) + .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( + lifecycle -> lifecycle.onComplete(new CompletionContext( + CompletionContext.Status.SUCCESS, lbResponse, clientResponse)))) + .map(clientResponse -> { + loadBalancerRetryContext.setClientResponse(clientResponse); + if (shouldRetrySameServiceInstance(loadBalancerRetryContext)) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("Retrying on status code: %d", + clientResponse.statusCode().value())); + } + throw new RetryableStatusCodeException(); } - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse))); - return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) - .body(serviceInstanceUnavailableMessage(serviceId)).build()); - } - - if (LOG.isDebugEnabled()) { - LOG.debug(String - .format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, - instance.getUri())); - } - ClientRequest newRequest = buildClientRequest(clientRequest, reconstructURI(instance, originalUrl)); - return next.exchange(newRequest) - .doOnError(throwable -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle - .onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse)))) - .doOnSuccess(clientResponse -> - supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle - .onComplete(new CompletionContext( - CompletionContext.Status.SUCCESS, lbResponse, clientResponse)))) - .map(clientResponse -> { - loadBalancerRetryContext.setClientResponse(clientResponse); - if (shouldRetrySameServiceInstance(loadBalancerRetryContext)) { - if (LOG.isDebugEnabled()) { - LOG.debug(String - .format("Retrying on status code: %d", clientResponse - .statusCode().value())); - } - throw new RetryableStatusCodeException(); - } - return clientResponse; - - }); - }) - .map(clientResponse -> { - loadBalancerRetryContext.setClientResponse(clientResponse); - if (shouldRetryNextServiceInstance(loadBalancerRetryContext)) { - if (LOG.isDebugEnabled()) { - LOG.debug(String - .format("Retrying on status code: %d", clientResponse - .statusCode().value())); - } - throw new RetryableStatusCodeException(); - } - return clientResponse; - - } - ) - .retryWhen(exchangeRetry) - ) - .retryWhen(filterRetry); + return clientResponse; + + }); + }).map(clientResponse -> { + loadBalancerRetryContext.setClientResponse(clientResponse); + if (shouldRetryNextServiceInstance(loadBalancerRetryContext)) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("Retrying on status code: %d", clientResponse.statusCode().value())); + } + throw new RetryableStatusCodeException(); + } + return clientResponse; + + }).retryWhen(exchangeRetry)).retryWhen(filterRetry); } @NotNull private Retry buildRetrySpec(int max, boolean transientErrors) { - LoadBalancerProperties.Retry.Backoff backoffProperties = properties.getRetry() - .getBackoff(); + LoadBalancerProperties.Retry.Backoff backoffProperties = properties.getRetry().getBackoff(); if (backoffProperties.isEnabled()) { - return RetrySpec - .backoff(max, backoffProperties.getMinBackoff()) - .filter(this::isRetryException) - .maxBackoff(backoffProperties.getMaxBackoff()) - .jitter(backoffProperties.getJitter()) + return RetrySpec.backoff(max, backoffProperties.getMinBackoff()).filter(this::isRetryException) + .maxBackoff(backoffProperties.getMaxBackoff()).jitter(backoffProperties.getJitter()) .transientErrors(transientErrors); } - return RetrySpec - .max(max) - .filter(this::isRetryException) - .transientErrors(transientErrors); + return RetrySpec.max(max).filter(this::isRetryException).transientErrors(transientErrors); } private boolean shouldRetrySameServiceInstance(LoadBalancerRetryContext loadBalancerRetryContext) { - boolean shouldRetry = retryPolicy - .retryableStatusCode(loadBalancerRetryContext.getResponseStatusCode()) - && retryPolicy - .canRetryOnMethod(loadBalancerRetryContext.getRequestMethod()) + boolean shouldRetry = retryPolicy.retryableStatusCode(loadBalancerRetryContext.getResponseStatusCode()) + && retryPolicy.canRetryOnMethod(loadBalancerRetryContext.getRequestMethod()) && retryPolicy.canRetrySameServiceInstance(loadBalancerRetryContext); if (shouldRetry) { loadBalancerRetryContext.incrementRetriesSameServiceInstance(); @@ -178,9 +173,8 @@ private boolean shouldRetrySameServiceInstance(LoadBalancerRetryContext loadBala } private boolean shouldRetryNextServiceInstance(LoadBalancerRetryContext loadBalancerRetryContext) { - boolean shouldRetry = retryPolicy - .retryableStatusCode(loadBalancerRetryContext.getResponseStatusCode()) && - retryPolicy.canRetryOnMethod(loadBalancerRetryContext.getRequestMethod()) + boolean shouldRetry = retryPolicy.retryableStatusCode(loadBalancerRetryContext.getResponseStatusCode()) + && retryPolicy.canRetryOnMethod(loadBalancerRetryContext.getRequestMethod()) && retryPolicy.canRetryNextServiceInstance(loadBalancerRetryContext); if (shouldRetry) { loadBalancerRetryContext.incrementRetriesNextServiceInstance(); @@ -191,40 +185,17 @@ private boolean shouldRetryNextServiceInstance(LoadBalancerRetryContext loadBala private boolean isRetryException(Throwable throwable) { return exceptions.stream() - .anyMatch(exception -> exception - .isInstance(throwable) || throwable != null && exception - .isInstance(throwable.getCause()) + .anyMatch(exception -> exception.isInstance(throwable) + || throwable != null && exception.isInstance(throwable.getCause()) || Exceptions.isRetryExhausted(throwable)); } - protected URI reconstructURI(ServiceInstance instance, URI original) { - return LoadBalancerUriTools.reconstructURI(instance, original); - } - protected Mono> choose(String serviceId, Request request) { - ReactiveLoadBalancer loadBalancer = loadBalancerFactory - .getInstance(serviceId); + ReactiveLoadBalancer loadBalancer = loadBalancerFactory.getInstance(serviceId); if (loadBalancer == null) { return Mono.just(new EmptyResponse()); } return Mono.from(loadBalancer.choose(request)); } - private String serviceInstanceUnavailableMessage(String serviceId) { - return "Load balancer does not contain an instance for the service " + serviceId; - } - - private ClientRequest buildClientRequest(ClientRequest request, URI uri) { - return ClientRequest.create(request.method(), uri) - .headers(headers -> headers.addAll(request.headers())) - .cookies(cookies -> cookies.addAll(request.cookies())) - .attributes(attributes -> attributes.putAll(request.attributes())) - .body(request.body()).build(); - } - - private String getHint(String serviceId) { - String defaultHint = properties.getHint().getOrDefault("default", "default"); - String hintPropertyValue = properties.getHint().get(serviceId); - return hintPropertyValue != null ? hintPropertyValue : defaultHint; - } } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java index 8ad5541a5..cab2a8ba2 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java @@ -1,7 +1,24 @@ +/* + * 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.reactive; /** * @author Olga Maciaszek-Sharma */ public class RetryableStatusCodeException extends IllegalStateException { + } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java index 74d07209c..d5f2b3f6e 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.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.client.loadbalancer.reactive; import java.util.List; @@ -45,11 +61,9 @@ public Publisher> choose(Request request) { List instances = discoveryClient.getInstances(serviceId); if (request.getContext() instanceof RetryableRequestContext) { - RetryableRequestContext context = (RetryableRequestContext) request - .getContext(); + RetryableRequestContext context = (RetryableRequestContext) request.getContext(); if (context.getPreviousServiceInstance() != null) { - List instancesCopy = discoveryClient - .getInstances(serviceId); + List instancesCopy = discoveryClient.getInstances(serviceId); instancesCopy.remove(context.getPreviousServiceInstance()); if (!instancesCopy.isEmpty()) { instances = instancesCopy; @@ -63,4 +77,4 @@ public Publisher> choose(Request request) { return Mono.just(new DefaultResponse(instances.get(instanceIdx))); } -} \ No newline at end of file +} diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java index 830bdede8..30887182a 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java @@ -239,4 +239,4 @@ protected String getName() { } -} \ No newline at end of file +} diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index d7c5f299b..a5765c2fa 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.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.client.loadbalancer.reactive; import java.net.URI; @@ -67,21 +83,41 @@ void setUp() { instance.setServiceId("testservice"); instance.setUri(URI.create("http://localhost:" + port)); DefaultServiceInstance instanceWithNoLifecycleProcessors = new DefaultServiceInstance(); - instanceWithNoLifecycleProcessors - .setServiceId("serviceWithNoLifecycleProcessors"); - instanceWithNoLifecycleProcessors - .setUri(URI.create("http://localhost:" + port)); + instanceWithNoLifecycleProcessors.setServiceId("serviceWithNoLifecycleProcessors"); + instanceWithNoLifecycleProcessors.setUri(URI.create("http://localhost:" + port)); properties.getInstances().put("testservice", Collections.singletonList(instance)); properties.getInstances().put("serviceWithNoLifecycleProcessors", Collections.singletonList(instanceWithNoLifecycleProcessors)); } + @Test + void loadBalancerLifecycleCallbacksExecuted() { + final String callbackTestHint = "callbackTestHint"; + loadBalancerProperties.getHint().put("testservice", "callbackTestHint"); + final String result = "callbackTestResult"; + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/callback").exchange().block(); + + Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() + .values(); + Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) + .getCompleteLog().values(); + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + .contains(callbackTestHint); + assertThat(anotherLifecycleLogRequests) + .extracting(completionContext -> ((ClientResponse) completionContext.getClientResponse()) + .bodyToMono(String.class).block()) + .contains(result); + } + @Test void correctResponseReturnedForExistingHostAndInstancePresent() { ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/hello") - .exchange() - .block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); @@ -93,9 +129,7 @@ void correctResponseReturnedAfterRetryingOnSameServiceInstance() { loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/exception") - .exchange() - .block(); + .filter(this.loadBalancerFunction).build().get().uri("/exception").exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World!"); @@ -110,32 +144,25 @@ void correctResponseReturnedAfterRetryingOnNextServiceInstance() { DefaultServiceInstance badRetryTestInstance = new DefaultServiceInstance(); badRetryTestInstance.setServiceId("retrytest"); badRetryTestInstance.setUri(URI.create("http://localhost:" + 8080)); - properties.getInstances().put("retrytest", - Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); + properties.getInstances().put("retrytest", Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); ClientResponse clientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello") - .exchange() - .block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); ClientResponse secondClientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello") - .exchange() - .block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); then(secondClientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(secondClientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); } - @Test void serviceUnavailableReturnedWhenNoInstancePresent() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx") - .filter(this.loadBalancerFunction) + ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx").filter(this.loadBalancerFunction) .build().get().exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); @@ -143,10 +170,9 @@ void serviceUnavailableReturnedWhenNoInstancePresent() { @Test @Disabled - // FIXME 3.0.0 + // FIXME 3.0.0 void badRequestReturnedForIncorrectHost() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx") - .filter(this.loadBalancerFunction) + ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx").filter(this.loadBalancerFunction) .build().get().exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); @@ -154,44 +180,14 @@ void badRequestReturnedForIncorrectHost() { @Test void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { - assertThatCode(() -> WebClient.builder() - .baseUrl("http://serviceWithNoLifecycleProcessors") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() - .block()) - .doesNotThrowAnyException(); + assertThatCode(() -> WebClient.builder().baseUrl("http://serviceWithNoLifecycleProcessors") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block()) + .doesNotThrowAnyException(); } - @Test - void loadBalancerLifecycleCallbacksExecuted() { - final String callbackTestHint = "callbackTestHint"; - loadBalancerProperties.getHint().put("testservice", "callbackTestHint"); - final String result = "callbackTestResult"; - ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/callback") - .exchange().block(); - - Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class) - .get("loadBalancerLifecycle")).getStartLog() - .values(); - Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class) - .get("anotherLoadBalancerLifecycle")) - .getCompleteLog().values(); - then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - assertThat(lifecycleLogRequests) - .extracting(request -> ((DefaultRequestContext) request.getContext()) - .getHint()) - .contains(callbackTestHint); - assertThat(anotherLifecycleLogRequests) - .extracting(completionContext -> ((ClientResponse) completionContext - .getClientResponse()) - .bodyToMono(String.class).block()) - .contains(result); - } - @SuppressWarnings({"unchecked", "rawtypes"}) + @SuppressWarnings({ "unchecked", "rawtypes" }) @EnableDiscoveryClient @EnableAutoConfiguration @SpringBootConfiguration(proxyBeanMethods = false) @@ -229,7 +225,8 @@ ReactiveLoadBalancer.Factory reactiveLoadBalancerFactory(Discov @Override public ReactiveLoadBalancer getInstance(String serviceId) { - return new org.springframework.cloud.client.loadbalancer.reactive.DiscoveryClientBasedReactiveLoadBalancer(serviceId, discoveryClient); + return new org.springframework.cloud.client.loadbalancer.reactive.DiscoveryClientBasedReactiveLoadBalancer( + serviceId, discoveryClient); } @Override @@ -238,10 +235,8 @@ public Map getInstances(String name, Class type) { return null; } Map lifecycleProcessors = new HashMap<>(); - lifecycleProcessors - .put("loadBalancerLifecycle", testLoadBalancerLifecycle); - lifecycleProcessors - .put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); + lifecycleProcessors.put("loadBalancerLifecycle", testLoadBalancerLifecycle); + lifecycleProcessors.put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); return lifecycleProcessors; } @@ -252,7 +247,6 @@ public X getInstance(String name, Class clazz, Class... generics) { }; } - @Bean LoadBalancerProperties loadBalancerProperties() { return new LoadBalancerProperties(); @@ -261,9 +255,10 @@ LoadBalancerProperties loadBalancerProperties() { @Bean RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerProperties properties, ReactiveLoadBalancer.Factory factory) { - return new RetryableLoadBalancerExchangeFilterFunction(new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), - properties, factory); + return new RetryableLoadBalancerExchangeFilterFunction( + new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), properties, factory); } + } protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { @@ -304,6 +299,5 @@ protected String getName() { } } -} - +} From 1b05fd12fac3e6851a59cdf3636f4534dce1df0b Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 12:03:57 +0100 Subject: [PATCH 06/16] Fix test. --- ...adBalancerExchangeFilterFunctionTests.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index a5765c2fa..259b59280 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -17,13 +17,15 @@ package org.springframework.cloud.client.loadbalancer.reactive; import java.net.URI; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.UUID; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentSkipListMap; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.BeforeEach; @@ -101,17 +103,17 @@ void loadBalancerLifecycleCallbacksExecuted() { Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() - .values(); - Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) - .getCompleteLog().values(); + .values(); + List> anotherLifecycleLogRequests = new ArrayList<>( + ((AnotherLoadBalancerLifecycle) factory.getInstances("testservice", LoadBalancerLifecycle.class) + .get("anotherLoadBalancerLifecycle")).getCompleteLog().values()); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) .contains(callbackTestHint); - assertThat(anotherLifecycleLogRequests) + assertThat(anotherLifecycleLogRequests.get(anotherLifecycleLogRequests.size() - 1)) .extracting(completionContext -> ((ClientResponse) completionContext.getClientResponse()) .bodyToMono(String.class).block()) - .contains(result); + .isEqualTo(result); } @Test @@ -185,8 +187,6 @@ void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { .doesNotThrowAnyException(); } - - @SuppressWarnings({ "unchecked", "rawtypes" }) @EnableDiscoveryClient @EnableAutoConfiguration @@ -263,9 +263,9 @@ RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerP protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { - ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); + Map> startLog = new ConcurrentSkipListMap<>(); - ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + Map> completeLog = new ConcurrentSkipListMap<>(); @Override public void onStart(Request request) { @@ -277,11 +277,11 @@ public void onComplete(CompletionContext completionCont completeLog.put(getName() + UUID.randomUUID(), completionContext); } - ConcurrentHashMap> getStartLog() { + Map> getStartLog() { return startLog; } - ConcurrentHashMap> getCompleteLog() { + Map> getCompleteLog() { return completeLog; } From 14b79e7ec6c7aaef7c26e58045bed225322eb927 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 14:08:12 +0100 Subject: [PATCH 07/16] Add more tests. --- ...xchangeFilterFunctionIntegrationTests.java | 304 +++++++++++++++++ ...adBalancerExchangeFilterFunctionTests.java | 312 +++++------------- 2 files changed, 378 insertions(+), 238 deletions(-) create mode 100644 spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java new file mode 100644 index 000000000..a2b19969d --- /dev/null +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java @@ -0,0 +1,304 @@ +/* + * 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.reactive; + +import java.net.URI; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.web.server.LocalServerPort; +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.client.discovery.EnableDiscoveryClient; +import org.springframework.cloud.client.discovery.simple.SimpleDiscoveryProperties; +import org.springframework.cloud.client.loadbalancer.CompletionContext; +import org.springframework.cloud.client.loadbalancer.DefaultRequestContext; +import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.context.annotation.Bean; +import org.springframework.http.HttpStatus; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.reactive.function.client.ClientResponse; +import org.springframework.web.reactive.function.client.WebClient; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.BDDAssertions.then; +import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; + +/** + * @author Olga Maciaszek-Sharma + */ +@SpringBootTest(webEnvironment = RANDOM_PORT) +class RetryableLoadBalancerExchangeFilterFunctionIntegrationTests { + + @Autowired + private RetryableLoadBalancerExchangeFilterFunction loadBalancerFunction; + + @Autowired + private SimpleDiscoveryProperties properties; + + @Autowired + private LoadBalancerProperties loadBalancerProperties; + + @Autowired + private ReactiveLoadBalancer.Factory factory; + + @LocalServerPort + private int port; + + @BeforeEach + void setUp() { + DefaultServiceInstance instance = new DefaultServiceInstance(); + instance.setServiceId("testservice"); + instance.setUri(URI.create("http://localhost:" + port)); + DefaultServiceInstance instanceWithNoLifecycleProcessors = new DefaultServiceInstance(); + instanceWithNoLifecycleProcessors.setServiceId("serviceWithNoLifecycleProcessors"); + instanceWithNoLifecycleProcessors.setUri(URI.create("http://localhost:" + port)); + properties.getInstances().put("testservice", Collections.singletonList(instance)); + properties.getInstances().put("serviceWithNoLifecycleProcessors", + Collections.singletonList(instanceWithNoLifecycleProcessors)); + } + + @Test + void loadBalancerLifecycleCallbacksExecuted() { + final String callbackTestHint = "callbackTestHint"; + loadBalancerProperties.getHint().put("testservice", "callbackTestHint"); + final String result = "callbackTestResult"; + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/callback").exchange().block(); + + Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() + .values(); + List> anotherLifecycleLogRequests = new ArrayList<>( + ((AnotherLoadBalancerLifecycle) factory.getInstances("testservice", LoadBalancerLifecycle.class) + .get("anotherLoadBalancerLifecycle")).getCompleteLog().values()); + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + .contains(callbackTestHint); + assertThat(anotherLifecycleLogRequests.get(anotherLifecycleLogRequests.size() - 1)) + .extracting(completionContext -> ((ClientResponse) completionContext.getClientResponse()) + .bodyToMono(String.class).block()) + .isEqualTo(result); + } + + @Test + void correctResponseReturnedForExistingHostAndInstancePresent() { + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + } + + @Test + void correctResponseReturnedAfterRetryingOnSameServiceInstance() { + loadBalancerProperties.getRetry().setMaxRetriesOnSameServiceInstance(1); + loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") + .filter(this.loadBalancerFunction).build().get().uri("/exception").exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World!"); + } + + @Test + void correctResponseReturnedAfterRetryingOnNextServiceInstanceWithBackoff() { + loadBalancerProperties.getRetry().getBackoff().setEnabled(true); + loadBalancerProperties.getRetry().setMaxRetriesOnSameServiceInstance(1); + DefaultServiceInstance goodRetryTestInstance = new DefaultServiceInstance(); + goodRetryTestInstance.setServiceId("retrytest"); + goodRetryTestInstance.setUri(URI.create("http://localhost:" + port)); + DefaultServiceInstance badRetryTestInstance = new DefaultServiceInstance(); + badRetryTestInstance.setServiceId("retrytest"); + badRetryTestInstance.setUri(URI.create("http://localhost:" + 8080)); + properties.getInstances().put("retrytest", Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); + loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); + + ClientResponse clientResponse = WebClient.builder().baseUrl("http://retrytest") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + + ClientResponse secondClientResponse = WebClient.builder().baseUrl("http://retrytest") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + + then(secondClientResponse.statusCode()).isEqualTo(HttpStatus.OK); + then(secondClientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + } + + @Test + void serviceUnavailableReturnedWhenNoInstancePresent() { + ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx").filter(this.loadBalancerFunction) + .build().get().exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); + } + + @Test + @Disabled + // FIXME 3.0.0 + void badRequestReturnedForIncorrectHost() { + ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx").filter(this.loadBalancerFunction) + .build().get().exchange().block(); + + then(clientResponse.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + } + + @Test + void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { + assertThatCode(() -> WebClient.builder().baseUrl("http://serviceWithNoLifecycleProcessors") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block()) + .doesNotThrowAnyException(); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + @EnableDiscoveryClient + @EnableAutoConfiguration + @SpringBootConfiguration(proxyBeanMethods = false) + @RestController + static class Config { + + AtomicInteger exceptionCallsCount = new AtomicInteger(); + + @GetMapping("/hello") + public String hello() { + return "Hello World"; + } + + @GetMapping("/callback") + String callbackTestResult() { + return "callbackTestResult"; + } + + @GetMapping("/exception") + String exception() { + int callCount = exceptionCallsCount.incrementAndGet(); + if (callCount % 2 != 0) { + throw new IllegalStateException("Test!"); + } + return "Hello World!"; + } + + @Bean + ReactiveLoadBalancer.Factory reactiveLoadBalancerFactory(DiscoveryClient discoveryClient) { + return new ReactiveLoadBalancer.Factory() { + + private final TestLoadBalancerLifecycle testLoadBalancerLifecycle = new TestLoadBalancerLifecycle(); + + private final TestLoadBalancerLifecycle anotherLoadBalancerLifecycle = new AnotherLoadBalancerLifecycle(); + + @Override + public ReactiveLoadBalancer getInstance(String serviceId) { + return new org.springframework.cloud.client.loadbalancer.reactive.DiscoveryClientBasedReactiveLoadBalancer( + serviceId, discoveryClient); + } + + @Override + public Map getInstances(String name, Class type) { + if (name.equals("serviceWithNoLifecycleProcessors")) { + return null; + } + Map lifecycleProcessors = new HashMap<>(); + lifecycleProcessors.put("loadBalancerLifecycle", testLoadBalancerLifecycle); + lifecycleProcessors.put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); + return lifecycleProcessors; + } + + @Override + public X getInstance(String name, Class clazz, Class... generics) { + return null; + } + }; + } + + @Bean + LoadBalancerProperties loadBalancerProperties() { + return new LoadBalancerProperties(); + } + + @Bean + RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerProperties properties, + ReactiveLoadBalancer.Factory factory) { + return new RetryableLoadBalancerExchangeFilterFunction( + new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), properties, factory); + } + + } + + protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { + + Map> startLog = new ConcurrentSkipListMap<>(); + + Map> completeLog = new ConcurrentSkipListMap<>(); + + @Override + public void onStart(Request request) { + startLog.put(getName() + UUID.randomUUID(), request); + } + + @Override + public void onComplete(CompletionContext completionContext) { + completeLog.put(getName() + UUID.randomUUID(), completionContext); + } + + Map> getStartLog() { + return startLog; + } + + Map> getCompleteLog() { + return completeLog; + } + + protected String getName() { + return this.getClass().getSimpleName(); + } + + } + + protected static class AnotherLoadBalancerLifecycle extends TestLoadBalancerLifecycle { + + @Override + protected String getName() { + return this.getClass().getSimpleName(); + } + + } + +} diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index 259b59280..b8b7c339a 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -16,288 +16,124 @@ package org.springframework.cloud.client.loadbalancer.reactive; +import java.io.IOException; import java.net.URI; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.UUID; -import java.util.concurrent.ConcurrentSkipListMap; -import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import reactor.core.publisher.Mono; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.SpringBootConfiguration; -import org.springframework.boot.autoconfigure.EnableAutoConfiguration; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.web.server.LocalServerPort; -import org.springframework.cloud.client.DefaultServiceInstance; import org.springframework.cloud.client.ServiceInstance; -import org.springframework.cloud.client.discovery.DiscoveryClient; -import org.springframework.cloud.client.discovery.EnableDiscoveryClient; -import org.springframework.cloud.client.discovery.simple.SimpleDiscoveryProperties; -import org.springframework.cloud.client.loadbalancer.CompletionContext; -import org.springframework.cloud.client.loadbalancer.DefaultRequestContext; -import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; -import org.springframework.cloud.client.loadbalancer.Request; -import org.springframework.context.annotation.Bean; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.reactive.function.client.ClientRequest; import org.springframework.web.reactive.function.client.ClientResponse; -import org.springframework.web.reactive.function.client.WebClient; +import org.springframework.web.reactive.function.client.ExchangeFunction; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatCode; -import static org.assertj.core.api.BDDAssertions.then; -import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * @author Olga Maciaszek-Sharma */ -@SpringBootTest(webEnvironment = RANDOM_PORT) -public class RetryableLoadBalancerExchangeFilterFunctionTests { +@SuppressWarnings("unchecked") +class RetryableLoadBalancerExchangeFilterFunctionTests { - @Autowired - private RetryableLoadBalancerExchangeFilterFunction loadBalancerFunction; + private final LoadBalancerProperties properties = new LoadBalancerProperties(); - @Autowired - private SimpleDiscoveryProperties properties; + private final LoadBalancerRetryPolicy policy = new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy( + properties); - @Autowired - private LoadBalancerProperties loadBalancerProperties; + private final ReactiveLoadBalancer.Factory factory = mock(ReactiveLoadBalancer.Factory.class); - @Autowired - private ReactiveLoadBalancer.Factory factory; + private final RetryableLoadBalancerExchangeFilterFunction filterFunction = new RetryableLoadBalancerExchangeFilterFunction( + policy, properties, factory); - @LocalServerPort - private int port; + private final ClientRequest clientRequest = mock(ClientRequest.class); - @BeforeEach - void setUp() { - DefaultServiceInstance instance = new DefaultServiceInstance(); - instance.setServiceId("testservice"); - instance.setUri(URI.create("http://localhost:" + port)); - DefaultServiceInstance instanceWithNoLifecycleProcessors = new DefaultServiceInstance(); - instanceWithNoLifecycleProcessors.setServiceId("serviceWithNoLifecycleProcessors"); - instanceWithNoLifecycleProcessors.setUri(URI.create("http://localhost:" + port)); - properties.getInstances().put("testservice", Collections.singletonList(instance)); - properties.getInstances().put("serviceWithNoLifecycleProcessors", - Collections.singletonList(instanceWithNoLifecycleProcessors)); - } + private final ExchangeFunction next = mock(ExchangeFunction.class); - @Test - void loadBalancerLifecycleCallbacksExecuted() { - final String callbackTestHint = "callbackTestHint"; - loadBalancerProperties.getHint().put("testservice", "callbackTestHint"); - final String result = "callbackTestResult"; - - ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/callback").exchange().block(); + private final ClientResponse clientResponse = mock(ClientResponse.class); - Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() - .values(); - List> anotherLifecycleLogRequests = new ArrayList<>( - ((AnotherLoadBalancerLifecycle) factory.getInstances("testservice", LoadBalancerLifecycle.class) - .get("anotherLoadBalancerLifecycle")).getCompleteLog().values()); - then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) - .contains(callbackTestHint); - assertThat(anotherLifecycleLogRequests.get(anotherLifecycleLogRequests.size() - 1)) - .extracting(completionContext -> ((ClientResponse) completionContext.getClientResponse()) - .bodyToMono(String.class).block()) - .isEqualTo(result); - } - - @Test - void correctResponseReturnedForExistingHostAndInstancePresent() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + @BeforeEach + void setUp() { + properties.getRetry().setMaxRetriesOnSameServiceInstance(1); + properties.getRetry().getRetryableStatusCodes().add(404); + when(clientRequest.url()).thenReturn(URI.create("http://test")); + when(factory.getInstance("test")).thenReturn(new TestReactiveLoadBalancer()); + when(clientRequest.headers()).thenReturn(new HttpHeaders()); + when(clientRequest.cookies()).thenReturn(new HttpHeaders()); - then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); } @Test - void correctResponseReturnedAfterRetryingOnSameServiceInstance() { - loadBalancerProperties.getRetry().setMaxRetriesOnSameServiceInstance(1); - loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); + void shouldRetryOnSameAndNextServiceInstanceOnException() { + when(clientRequest.method()).thenReturn(HttpMethod.GET); + when(next.exchange(any())).thenThrow(new IllegalStateException(new IOException())); - ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/exception").exchange().block(); + filterFunction.filter(clientRequest, next).subscribe(); - then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World!"); + verify(next, times(4)).exchange(any()); + verify(factory, times(2)).getInstance(any()); } @Test - void correctResponseReturnedAfterRetryingOnNextServiceInstance() { - loadBalancerProperties.getRetry().setMaxRetriesOnSameServiceInstance(1); - DefaultServiceInstance goodRetryTestInstance = new DefaultServiceInstance(); - goodRetryTestInstance.setServiceId("retrytest"); - goodRetryTestInstance.setUri(URI.create("http://localhost:" + port)); - DefaultServiceInstance badRetryTestInstance = new DefaultServiceInstance(); - badRetryTestInstance.setServiceId("retrytest"); - badRetryTestInstance.setUri(URI.create("http://localhost:" + 8080)); - properties.getInstances().put("retrytest", Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); - loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); - - ClientResponse clientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + void shouldRetryOnSameAndNextServiceInstanceOnRetryableStatusCode() { + when(clientRequest.method()).thenReturn(HttpMethod.GET); + when(clientResponse.statusCode()).thenReturn(HttpStatus.NOT_FOUND); + when(next.exchange(any())).thenReturn(Mono.just(clientResponse)); - then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + filterFunction.filter(clientRequest, next).subscribe(); - ClientResponse secondClientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); - - then(secondClientResponse.statusCode()).isEqualTo(HttpStatus.OK); - then(secondClientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + verify(next, times(4)).exchange(any()); + verify(factory, times(2)).getInstance(any()); } @Test - void serviceUnavailableReturnedWhenNoInstancePresent() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx").filter(this.loadBalancerFunction) - .build().get().exchange().block(); + void shouldNotRetryWhenNoRetryableExceptionOrStatusCode() { + when(clientRequest.method()).thenReturn(HttpMethod.GET); + when(clientResponse.statusCode()).thenReturn(HttpStatus.OK); + when(next.exchange(any())).thenReturn(Mono.just(clientResponse)); - then(clientResponse.statusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); - } + filterFunction.filter(clientRequest, next).subscribe(); - @Test - @Disabled - // FIXME 3.0.0 - void badRequestReturnedForIncorrectHost() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx").filter(this.loadBalancerFunction) - .build().get().exchange().block(); - - then(clientResponse.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + verify(next, times(1)).exchange(any()); + verify(factory, times(1)).getInstance(any()); } @Test - void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { - assertThatCode(() -> WebClient.builder().baseUrl("http://serviceWithNoLifecycleProcessors") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block()) - .doesNotThrowAnyException(); - } - - @SuppressWarnings({ "unchecked", "rawtypes" }) - @EnableDiscoveryClient - @EnableAutoConfiguration - @SpringBootConfiguration(proxyBeanMethods = false) - @RestController - static class Config { - - AtomicInteger exceptionCallsCount = new AtomicInteger(); - - @GetMapping("/hello") - public String hello() { - return "Hello World"; - } - - @GetMapping("/callback") - String callbackTestResult() { - return "callbackTestResult"; - } - - @GetMapping("/exception") - String exception() { - int callCount = exceptionCallsCount.incrementAndGet(); - if (callCount % 2 != 0) { - throw new IllegalStateException("Test!"); - } - return "Hello World!"; - } + void shouldNotRetryOnMethodOtherThanGet() { + when(clientRequest.method()).thenReturn(HttpMethod.POST); + when(clientResponse.statusCode()).thenReturn(HttpStatus.NOT_FOUND); + when(next.exchange(any())).thenReturn(Mono.just(clientResponse)); - @Bean - ReactiveLoadBalancer.Factory reactiveLoadBalancerFactory(DiscoveryClient discoveryClient) { - return new ReactiveLoadBalancer.Factory() { - - private final TestLoadBalancerLifecycle testLoadBalancerLifecycle = new TestLoadBalancerLifecycle(); - - private final TestLoadBalancerLifecycle anotherLoadBalancerLifecycle = new AnotherLoadBalancerLifecycle(); - - @Override - public ReactiveLoadBalancer getInstance(String serviceId) { - return new org.springframework.cloud.client.loadbalancer.reactive.DiscoveryClientBasedReactiveLoadBalancer( - serviceId, discoveryClient); - } - - @Override - public Map getInstances(String name, Class type) { - if (name.equals("serviceWithNoLifecycleProcessors")) { - return null; - } - Map lifecycleProcessors = new HashMap<>(); - lifecycleProcessors.put("loadBalancerLifecycle", testLoadBalancerLifecycle); - lifecycleProcessors.put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); - return lifecycleProcessors; - } - - @Override - public X getInstance(String name, Class clazz, Class... generics) { - return null; - } - }; - } - - @Bean - LoadBalancerProperties loadBalancerProperties() { - return new LoadBalancerProperties(); - } - - @Bean - RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerProperties properties, - ReactiveLoadBalancer.Factory factory) { - return new RetryableLoadBalancerExchangeFilterFunction( - new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), properties, factory); - } - - } - - protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { - - Map> startLog = new ConcurrentSkipListMap<>(); - - Map> completeLog = new ConcurrentSkipListMap<>(); - - @Override - public void onStart(Request request) { - startLog.put(getName() + UUID.randomUUID(), request); - } - - @Override - public void onComplete(CompletionContext completionContext) { - completeLog.put(getName() + UUID.randomUUID(), completionContext); - } - - Map> getStartLog() { - return startLog; - } - - Map> getCompleteLog() { - return completeLog; - } - - protected String getName() { - return this.getClass().getSimpleName(); - } + filterFunction.filter(clientRequest, next).subscribe(); + verify(next, times(1)).exchange(any()); + verify(factory, times(1)).getInstance(any()); } - protected static class AnotherLoadBalancerLifecycle extends TestLoadBalancerLifecycle { - - @Override - protected String getName() { - return this.getClass().getSimpleName(); - } - + @Test + void shouldRetryOnMethodOtherThanGetWhenEnabled() { + LoadBalancerProperties properties = new LoadBalancerProperties(); + properties.getRetry().setRetryOnAllOperations(true); + properties.getRetry().setMaxRetriesOnSameServiceInstance(1); + properties.getRetry().getRetryableStatusCodes().add(404); + LoadBalancerRetryPolicy policy = new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties); + RetryableLoadBalancerExchangeFilterFunction filterFunction = new RetryableLoadBalancerExchangeFilterFunction( + policy, properties, factory); + when(clientRequest.method()).thenReturn(HttpMethod.POST); + when(clientResponse.statusCode()).thenReturn(HttpStatus.NOT_FOUND); + when(next.exchange(any())).thenReturn(Mono.just(clientResponse)); + + filterFunction.filter(clientRequest, next).subscribe(); + + verify(next, times(4)).exchange(any()); + verify(factory, times(2)).getInstance(any()); } } From 32bb645eb25660e91f8a1342406e398e459231b1 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 14:42:00 +0100 Subject: [PATCH 08/16] Fix test. --- ...xchangeFilterFunctionIntegrationTests.java | 83 ++++++++++++------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java index a2b19969d..849f05611 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java @@ -17,15 +17,13 @@ package org.springframework.cloud.client.loadbalancer.reactive; import java.net.URI; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.UUID; -import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.BeforeEach; @@ -85,7 +83,8 @@ void setUp() { instance.setServiceId("testservice"); instance.setUri(URI.create("http://localhost:" + port)); DefaultServiceInstance instanceWithNoLifecycleProcessors = new DefaultServiceInstance(); - instanceWithNoLifecycleProcessors.setServiceId("serviceWithNoLifecycleProcessors"); + instanceWithNoLifecycleProcessors + .setServiceId("serviceWithNoLifecycleProcessors"); instanceWithNoLifecycleProcessors.setUri(URI.create("http://localhost:" + port)); properties.getInstances().put("testservice", Collections.singletonList(instance)); properties.getInstances().put("serviceWithNoLifecycleProcessors", @@ -99,27 +98,34 @@ void loadBalancerLifecycleCallbacksExecuted() { final String result = "callbackTestResult"; ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/callback").exchange().block(); + .filter(this.loadBalancerFunction).build().get().uri("/callback") + .exchange().block(); Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() - .values(); - List> anotherLifecycleLogRequests = new ArrayList<>( - ((AnotherLoadBalancerLifecycle) factory.getInstances("testservice", LoadBalancerLifecycle.class) - .get("anotherLoadBalancerLifecycle")).getCompleteLog().values()); + .getInstances("testservice", LoadBalancerLifecycle.class) + .get("loadBalancerLifecycle")).getStartLog() + .values(); + Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class) + .get("anotherLoadBalancerLifecycle")) + .getCompleteLog().values(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + assertThat(lifecycleLogRequests) + .extracting(request -> ((DefaultRequestContext) request.getContext()) + .getHint()) .contains(callbackTestHint); - assertThat(anotherLifecycleLogRequests.get(anotherLifecycleLogRequests.size() - 1)) - .extracting(completionContext -> ((ClientResponse) completionContext.getClientResponse()) + assertThat(anotherLifecycleLogRequests) + .extracting(completionContext -> ((ClientResponse) completionContext + .getClientResponse()) .bodyToMono(String.class).block()) - .isEqualTo(result); + .contains(result); } @Test void correctResponseReturnedForExistingHostAndInstancePresent() { ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() + .block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); @@ -131,7 +137,8 @@ void correctResponseReturnedAfterRetryingOnSameServiceInstance() { loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/exception").exchange().block(); + .filter(this.loadBalancerFunction).build().get().uri("/exception") + .exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World!"); @@ -147,25 +154,31 @@ void correctResponseReturnedAfterRetryingOnNextServiceInstanceWithBackoff() { DefaultServiceInstance badRetryTestInstance = new DefaultServiceInstance(); badRetryTestInstance.setServiceId("retrytest"); badRetryTestInstance.setUri(URI.create("http://localhost:" + 8080)); - properties.getInstances().put("retrytest", Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); + properties.getInstances().put("retrytest", Arrays + .asList(badRetryTestInstance, goodRetryTestInstance)); loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); ClientResponse clientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() + .block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); - ClientResponse secondClientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); + ClientResponse secondClientResponse = WebClient.builder() + .baseUrl("http://retrytest") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() + .block(); then(secondClientResponse.statusCode()).isEqualTo(HttpStatus.OK); - then(secondClientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); + then(secondClientResponse.bodyToMono(String.class).block()) + .isEqualTo("Hello World"); } @Test void serviceUnavailableReturnedWhenNoInstancePresent() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx").filter(this.loadBalancerFunction) + ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx") + .filter(this.loadBalancerFunction) .build().get().exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); @@ -173,9 +186,10 @@ void serviceUnavailableReturnedWhenNoInstancePresent() { @Test @Disabled - // FIXME 3.0.0 + // FIXME 3.0.0 void badRequestReturnedForIncorrectHost() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx").filter(this.loadBalancerFunction) + ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx") + .filter(this.loadBalancerFunction) .build().get().exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); @@ -183,12 +197,14 @@ void badRequestReturnedForIncorrectHost() { @Test void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { - assertThatCode(() -> WebClient.builder().baseUrl("http://serviceWithNoLifecycleProcessors") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block()) - .doesNotThrowAnyException(); + assertThatCode(() -> WebClient.builder() + .baseUrl("http://serviceWithNoLifecycleProcessors") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() + .block()) + .doesNotThrowAnyException(); } - @SuppressWarnings({ "unchecked", "rawtypes" }) + @SuppressWarnings({"unchecked", "rawtypes"}) @EnableDiscoveryClient @EnableAutoConfiguration @SpringBootConfiguration(proxyBeanMethods = false) @@ -236,8 +252,10 @@ public Map getInstances(String name, Class type) { return null; } Map lifecycleProcessors = new HashMap<>(); - lifecycleProcessors.put("loadBalancerLifecycle", testLoadBalancerLifecycle); - lifecycleProcessors.put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); + lifecycleProcessors + .put("loadBalancerLifecycle", testLoadBalancerLifecycle); + lifecycleProcessors + .put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); return lifecycleProcessors; } @@ -264,9 +282,9 @@ RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerP protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { - Map> startLog = new ConcurrentSkipListMap<>(); + Map> startLog = new ConcurrentHashMap<>(); - Map> completeLog = new ConcurrentSkipListMap<>(); + Map> completeLog = new ConcurrentHashMap<>(); @Override public void onStart(Request request) { @@ -275,6 +293,7 @@ public void onStart(Request request) { @Override public void onComplete(CompletionContext completionContext) { + completeLog.clear(); completeLog.put(getName() + UUID.randomUUID(), completionContext); } From 9de5b572df57a442e89f0022f6239f7e629522f4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 15:40:38 +0100 Subject: [PATCH 09/16] Add autoConfiguration. --- .../LoadBalancedExchangeFilterFunction.java | 26 +++++++ ...cerBeanPostProcessorAutoConfiguration.java | 4 +- ...orLoadBalancerClientAutoConfiguration.java | 22 +++++- ...torLoadBalancerExchangeFilterFunction.java | 2 +- ...bleLoadBalancerExchangeFilterFunction.java | 19 +++-- .../reactive/LoadBalancerTestUtils.java | 2 +- ...dBalancerClientAutoConfigurationTests.java | 39 +++++++++++ ...xchangeFilterFunctionIntegrationTests.java | 70 +++++++------------ ...adBalancerExchangeFilterFunctionTests.java | 4 +- 9 files changed, 126 insertions(+), 62 deletions(-) create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java new file mode 100644 index 000000000..91ca3c4fe --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java @@ -0,0 +1,26 @@ +/* + * 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.reactive; + +import org.springframework.web.reactive.function.client.ExchangeFilterFunction; + +/** + * @author Olga Maciaszek-Sharma + */ +public interface LoadBalancedExchangeFilterFunction extends ExchangeFilterFunction { + +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerBeanPostProcessorAutoConfiguration.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerBeanPostProcessorAutoConfiguration.java index 3af428078..8e61f33c0 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerBeanPostProcessorAutoConfiguration.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerBeanPostProcessorAutoConfiguration.java @@ -58,8 +58,8 @@ protected static class ReactorDeferringLoadBalancerFilterConfig { @Bean @Primary - DeferringLoadBalancerExchangeFilterFunction reactorDeferringLoadBalancerExchangeFilterFunction( - ObjectProvider exchangeFilterFunctionProvider) { + DeferringLoadBalancerExchangeFilterFunction reactorDeferringLoadBalancerExchangeFilterFunction( + ObjectProvider exchangeFilterFunctionProvider) { return new DeferringLoadBalancerExchangeFilterFunction<>(exchangeFilterFunctionProvider); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfiguration.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfiguration.java index 3a994e2de..3af26e461 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfiguration.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfiguration.java @@ -19,6 +19,8 @@ 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.ServiceInstance; import org.springframework.cloud.client.loadbalancer.LoadBalanced; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -38,10 +40,28 @@ public class ReactorLoadBalancerClientAutoConfiguration { @ConditionalOnMissingBean + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "false", + matchIfMissing = true) @Bean public ReactorLoadBalancerExchangeFilterFunction loadBalancerExchangeFilterFunction( - ReactiveLoadBalancer.Factory loadBalancerFactory, LoadBalancerProperties properties) { + ReactiveLoadBalancer.Factory loadBalancerFactory, LoadBalancerProperties properties) { return new ReactorLoadBalancerExchangeFilterFunction(loadBalancerFactory, properties); } + @ConditionalOnMissingBean + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true") + @Bean + public RetryableLoadBalancerExchangeFilterFunction retryableLoadBalancerExchangeFilterFunction( + ReactiveLoadBalancer.Factory loadBalancerFactory, LoadBalancerProperties properties, + LoadBalancerRetryPolicy retryPolicy) { + return new RetryableLoadBalancerExchangeFilterFunction(retryPolicy, loadBalancerFactory, properties); + } + + @ConditionalOnMissingBean + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true") + @Bean + public LoadBalancerRetryPolicy loadBalancerRetryPolicy(LoadBalancerProperties properties) { + return new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties); + } + } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index f8a7948a4..2d53c10dd 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -51,7 +51,7 @@ * @since 2.2.0 */ @SuppressWarnings({ "rawtypes", "unchecked" }) -public class ReactorLoadBalancerExchangeFilterFunction implements ExchangeFilterFunction { +public class ReactorLoadBalancerExchangeFilterFunction implements LoadBalancedExchangeFilterFunction { private static final Log LOG = LogFactory.getLog(ReactorLoadBalancerExchangeFilterFunction.class); diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index 6ad22a1de..769836c34 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -25,7 +25,6 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.jetbrains.annotations.NotNull; import reactor.core.Exceptions; import reactor.core.publisher.Mono; import reactor.util.retry.Retry; @@ -44,7 +43,6 @@ import org.springframework.http.HttpStatus; import org.springframework.web.reactive.function.client.ClientRequest; import org.springframework.web.reactive.function.client.ClientResponse; -import org.springframework.web.reactive.function.client.ExchangeFilterFunction; import org.springframework.web.reactive.function.client.ExchangeFunction; import static org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools.reconstructURI; @@ -55,13 +53,15 @@ /** * @author Olga Maciaszek-Sharma */ -public class RetryableLoadBalancerExchangeFilterFunction implements ExchangeFilterFunction { +public class RetryableLoadBalancerExchangeFilterFunction implements LoadBalancedExchangeFilterFunction { - private static final Log LOG = LogFactory.getLog(RetryableLoadBalancerExchangeFilterFunction.class); + private static final Log LOG = LogFactory + .getLog(RetryableLoadBalancerExchangeFilterFunction.class); - private static final List> exceptions = Arrays.asList(IOException.class, - TimeoutException.class, - org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); + private static final List> exceptions = Arrays + .asList(IOException.class, + TimeoutException.class, + org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); private final LoadBalancerRetryPolicy retryPolicy; @@ -70,10 +70,10 @@ public class RetryableLoadBalancerExchangeFilterFunction implements ExchangeFilt private final ReactiveLoadBalancer.Factory loadBalancerFactory; public RetryableLoadBalancerExchangeFilterFunction(LoadBalancerRetryPolicy retryPolicy, - LoadBalancerProperties properties, ReactiveLoadBalancer.Factory loadBalancerFactory) { + ReactiveLoadBalancer.Factory loadBalancerFactory, LoadBalancerProperties properties) { this.retryPolicy = retryPolicy; - this.properties = properties; this.loadBalancerFactory = loadBalancerFactory; + this.properties = properties; } @SuppressWarnings({ "rawtypes", "unchecked" }) @@ -151,7 +151,6 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction }).retryWhen(exchangeRetry)).retryWhen(filterRetry); } - @NotNull private Retry buildRetrySpec(int max, boolean transientErrors) { LoadBalancerProperties.Retry.Backoff backoffProperties = properties.getRetry().getBackoff(); if (backoffProperties.isEnabled()) { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java index 31311fb79..a8abc7e23 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java @@ -43,7 +43,7 @@ private LoadBalancerTestUtils() { static ConfigurableApplicationContext init(Class... configClasses) { return new SpringApplicationBuilder().web(WebApplicationType.NONE) - .sources(ArrayUtils.add(configClasses, WebClientAutoConfiguration.class)).run(); + .sources(ArrayUtils.add(configClasses, WebClientAutoConfiguration.class)).properties().run(); } @SuppressWarnings("unchecked") diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfigurationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfigurationTests.java index cac43fc80..455d40d5b 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfigurationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfigurationTests.java @@ -59,6 +59,27 @@ void loadBalancerFilterAddedToWebClientBuilder() { assertLoadBalanced(testService.webClient, ReactorLoadBalancerExchangeFilterFunction.class); } + @Test + void loadBalancerFilterAddedToWebClientBuilderWithRetryEnabled() { + System.setProperty("spring.cloud.loadbalancer.retry.enabled", "true"); + ConfigurableApplicationContext context = init(OneWebClientBuilder.class); + final Map webClientBuilders = context.getBeansOfType(WebClient.Builder.class); + + then(webClientBuilders).isNotNull().hasSize(1); + WebClient.Builder webClientBuilder = webClientBuilders.values().iterator().next(); + then(webClientBuilder).isNotNull(); + + assertLoadBalanced(webClientBuilder, RetryableLoadBalancerExchangeFilterFunction.class); + + final Map testServiceMap = context + .getBeansOfType(OneWebClientBuilder.TestService.class); + then(testServiceMap).isNotNull().hasSize(1); + OneWebClientBuilder.TestService testService = testServiceMap.values().stream().findFirst().get(); + assertLoadBalanced(testService.webClient, RetryableLoadBalancerExchangeFilterFunction.class); + + System.clearProperty("spring.cloud.loadbalancer.retry.enabled"); + } + @Test void loadBalancerFilterAddedOnlyToLoadBalancedWebClientBuilder() { ConfigurableApplicationContext context = init(TwoWebClientBuilders.class); @@ -75,6 +96,24 @@ void loadBalancerFilterAddedOnlyToLoadBalancedWebClientBuilder() { then(getFilters(two.nonLoadBalanced)).isNullOrEmpty(); } + @Test + void loadBalancerFilterAddedOnlyToLoadBalancedWebClientBuilderWithRetryEnabled() { + System.setProperty("spring.cloud.loadbalancer.retry.enabled", "true"); + ConfigurableApplicationContext context = init(TwoWebClientBuilders.class); + final Map webClientBuilders = context.getBeansOfType(WebClient.Builder.class); + + then(webClientBuilders).hasSize(2); + + TwoWebClientBuilders.Two two = context.getBean(TwoWebClientBuilders.Two.class); + + then(two.loadBalanced).isNotNull(); + assertLoadBalanced(two.loadBalanced, RetryableLoadBalancerExchangeFilterFunction.class); + + then(two.nonLoadBalanced).isNotNull(); + then(getFilters(two.nonLoadBalanced)).isNullOrEmpty(); + System.clearProperty("spring.cloud.loadbalancer.retry.enabled"); + } + @Test void noCustomWebClientBuilders() { ConfigurableApplicationContext context = init(NoWebClientBuilder.class); diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java index 849f05611..5267f3648 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java @@ -83,8 +83,7 @@ void setUp() { instance.setServiceId("testservice"); instance.setUri(URI.create("http://localhost:" + port)); DefaultServiceInstance instanceWithNoLifecycleProcessors = new DefaultServiceInstance(); - instanceWithNoLifecycleProcessors - .setServiceId("serviceWithNoLifecycleProcessors"); + instanceWithNoLifecycleProcessors.setServiceId("serviceWithNoLifecycleProcessors"); instanceWithNoLifecycleProcessors.setUri(URI.create("http://localhost:" + port)); properties.getInstances().put("testservice", Collections.singletonList(instance)); properties.getInstances().put("serviceWithNoLifecycleProcessors", @@ -98,25 +97,19 @@ void loadBalancerLifecycleCallbacksExecuted() { final String result = "callbackTestResult"; ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/callback") - .exchange().block(); + .filter(this.loadBalancerFunction).build().get().uri("/callback").exchange().block(); Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class) - .get("loadBalancerLifecycle")).getStartLog() - .values(); + .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() + .values(); Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory - .getInstances("testservice", LoadBalancerLifecycle.class) - .get("anotherLoadBalancerLifecycle")) - .getCompleteLog().values(); + .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) + .getCompleteLog().values(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); - assertThat(lifecycleLogRequests) - .extracting(request -> ((DefaultRequestContext) request.getContext()) - .getHint()) + assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) .contains(callbackTestHint); assertThat(anotherLifecycleLogRequests) - .extracting(completionContext -> ((ClientResponse) completionContext - .getClientResponse()) + .extracting(completionContext -> ((ClientResponse) completionContext.getClientResponse()) .bodyToMono(String.class).block()) .contains(result); } @@ -124,8 +117,7 @@ void loadBalancerLifecycleCallbacksExecuted() { @Test void correctResponseReturnedForExistingHostAndInstancePresent() { ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() - .block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); @@ -137,8 +129,7 @@ void correctResponseReturnedAfterRetryingOnSameServiceInstance() { loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice") - .filter(this.loadBalancerFunction).build().get().uri("/exception") - .exchange().block(); + .filter(this.loadBalancerFunction).build().get().uri("/exception").exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World!"); @@ -154,31 +145,25 @@ void correctResponseReturnedAfterRetryingOnNextServiceInstanceWithBackoff() { DefaultServiceInstance badRetryTestInstance = new DefaultServiceInstance(); badRetryTestInstance.setServiceId("retrytest"); badRetryTestInstance.setUri(URI.create("http://localhost:" + 8080)); - properties.getInstances().put("retrytest", Arrays - .asList(badRetryTestInstance, goodRetryTestInstance)); + properties.getInstances().put("retrytest", Arrays.asList(badRetryTestInstance, goodRetryTestInstance)); loadBalancerProperties.getRetry().getRetryableStatusCodes().add(500); ClientResponse clientResponse = WebClient.builder().baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() - .block(); + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); then(clientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); - ClientResponse secondClientResponse = WebClient.builder() - .baseUrl("http://retrytest") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() - .block(); + ClientResponse secondClientResponse = WebClient.builder().baseUrl("http://retrytest") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block(); then(secondClientResponse.statusCode()).isEqualTo(HttpStatus.OK); - then(secondClientResponse.bodyToMono(String.class).block()) - .isEqualTo("Hello World"); + then(secondClientResponse.bodyToMono(String.class).block()).isEqualTo("Hello World"); } @Test void serviceUnavailableReturnedWhenNoInstancePresent() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx") - .filter(this.loadBalancerFunction) + ClientResponse clientResponse = WebClient.builder().baseUrl("http://xxx").filter(this.loadBalancerFunction) .build().get().exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); @@ -186,10 +171,9 @@ void serviceUnavailableReturnedWhenNoInstancePresent() { @Test @Disabled - // FIXME 3.0.0 + // FIXME 3.0.0 void badRequestReturnedForIncorrectHost() { - ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx") - .filter(this.loadBalancerFunction) + ClientResponse clientResponse = WebClient.builder().baseUrl("http:///xxx").filter(this.loadBalancerFunction) .build().get().exchange().block(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); @@ -197,14 +181,12 @@ void badRequestReturnedForIncorrectHost() { @Test void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { - assertThatCode(() -> WebClient.builder() - .baseUrl("http://serviceWithNoLifecycleProcessors") - .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange() - .block()) - .doesNotThrowAnyException(); + assertThatCode(() -> WebClient.builder().baseUrl("http://serviceWithNoLifecycleProcessors") + .filter(this.loadBalancerFunction).build().get().uri("/hello").exchange().block()) + .doesNotThrowAnyException(); } - @SuppressWarnings({"unchecked", "rawtypes"}) + @SuppressWarnings({ "unchecked", "rawtypes" }) @EnableDiscoveryClient @EnableAutoConfiguration @SpringBootConfiguration(proxyBeanMethods = false) @@ -252,10 +234,8 @@ public Map getInstances(String name, Class type) { return null; } Map lifecycleProcessors = new HashMap<>(); - lifecycleProcessors - .put("loadBalancerLifecycle", testLoadBalancerLifecycle); - lifecycleProcessors - .put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); + lifecycleProcessors.put("loadBalancerLifecycle", testLoadBalancerLifecycle); + lifecycleProcessors.put("anotherLoadBalancerLifecycle", anotherLoadBalancerLifecycle); return lifecycleProcessors; } @@ -275,7 +255,7 @@ LoadBalancerProperties loadBalancerProperties() { RetryableLoadBalancerExchangeFilterFunction exchangeFilterFunction(LoadBalancerProperties properties, ReactiveLoadBalancer.Factory factory) { return new RetryableLoadBalancerExchangeFilterFunction( - new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), properties, factory); + new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties), factory, properties); } } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index b8b7c339a..30ff6f7e2 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -51,7 +51,7 @@ class RetryableLoadBalancerExchangeFilterFunctionTests { private final ReactiveLoadBalancer.Factory factory = mock(ReactiveLoadBalancer.Factory.class); private final RetryableLoadBalancerExchangeFilterFunction filterFunction = new RetryableLoadBalancerExchangeFilterFunction( - policy, properties, factory); + policy, factory, properties); private final ClientRequest clientRequest = mock(ClientRequest.class); @@ -125,7 +125,7 @@ void shouldRetryOnMethodOtherThanGetWhenEnabled() { properties.getRetry().getRetryableStatusCodes().add(404); LoadBalancerRetryPolicy policy = new RetryableExchangeFilterFunctionLoadBalancerRetryPolicy(properties); RetryableLoadBalancerExchangeFilterFunction filterFunction = new RetryableLoadBalancerExchangeFilterFunction( - policy, properties, factory); + policy, factory, properties); when(clientRequest.method()).thenReturn(HttpMethod.POST); when(clientResponse.statusCode()).thenReturn(HttpStatus.NOT_FOUND); when(next.exchange(any())).thenReturn(Mono.just(clientResponse)); From 079e32e006894e3a62f51cc92121a0033201c314 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 16:12:05 +0100 Subject: [PATCH 10/16] Refactor and add javadocs. --- .../reactive/ExchangeFilterFunctionTools.java | 4 +++ .../LoadBalancedExchangeFilterFunction.java | 3 ++ .../reactive/LoadBalancerProperties.java | 29 +++++++++------ .../reactive/LoadBalancerRetryContext.java | 5 ++- .../reactive/LoadBalancerRetryPolicy.java | 35 ++++++++++--------- ...FilterFunctionLoadBalancerRetryPolicy.java | 3 ++ ...bleLoadBalancerExchangeFilterFunction.java | 17 +++++---- .../RetryableStatusCodeException.java | 6 +++- ...coveryClientBasedReactiveLoadBalancer.java | 3 ++ .../reactive/LoadBalancerTestUtils.java | 2 +- ...xchangeFilterFunctionIntegrationTests.java | 3 ++ ...adBalancerExchangeFilterFunctionTests.java | 3 ++ 12 files changed, 76 insertions(+), 37 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java index a94b9cf74..da8fcbe69 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java @@ -20,9 +20,13 @@ import java.util.Map; import org.springframework.web.reactive.function.client.ClientRequest; +import org.springframework.web.reactive.function.client.ExchangeFilterFunction; /** + * A utility class for load-balanced {@link ExchangeFilterFunction} instances. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public final class ExchangeFilterFunctionTools { diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java index 91ca3c4fe..a6ba1d031 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancedExchangeFilterFunction.java @@ -19,7 +19,10 @@ import org.springframework.web.reactive.function.client.ExchangeFilterFunction; /** + * A marker interface for load-balanced {@link ExchangeFilterFunction} instances. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public interface LoadBalancedExchangeFilterFunction extends ExchangeFilterFunction { 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 7aae1799a..c25a43091 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 @@ -21,6 +21,8 @@ import java.util.Map; import java.util.Set; +import reactor.util.retry.RetryBackoffSpec; + import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.http.HttpMethod; import org.springframework.util.LinkedCaseInsensitiveMap; @@ -47,7 +49,7 @@ public class LoadBalancerProperties { private Map hint = new LinkedCaseInsensitiveMap<>(); /** - * Properties for Spring-Retry support in Spring Cloud LoadBalancer. + * Properties for Spring-Retry and Reactor Retry support in Spring Cloud LoadBalancer. */ private Retry retry = new Retry(); @@ -141,6 +143,9 @@ public static class Retry { */ private Set retryableStatusCodes = new HashSet<>(); + /** + * Properties for Reactor Retry backoffs in Spring Cloud LoadBalancer. + */ private Backoff backoff = new Backoff(); /** @@ -202,16 +207,26 @@ public void setBackoff(Backoff backoff) { public static class Backoff { + /** + * Indicates whether Reactor Retry backoffs should be applied. + */ private boolean enabled = false; + /** + * Used to set {@link RetryBackoffSpec#minBackoff}. + */ private Duration minBackoff = Duration.ofMillis(5); + /** + * Used to set {@link RetryBackoffSpec#maxBackoff}. + */ private Duration maxBackoff = Duration.ofMillis(Long.MAX_VALUE); + /** + * Used to set {@link RetryBackoffSpec#jitter}. + */ private double jitter = 0.5d; - private boolean basedOnPreviousValue = true; - public Duration getMinBackoff() { return minBackoff; } @@ -236,14 +251,6 @@ public void setJitter(double jitter) { this.jitter = jitter; } - public boolean isBasedOnPreviousValue() { - return basedOnPreviousValue; - } - - public void setBasedOnPreviousValue(boolean basedOnPreviousValue) { - this.basedOnPreviousValue = basedOnPreviousValue; - } - public boolean isEnabled() { return enabled; } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java index 29a1b8d12..f6e55c776 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryContext.java @@ -21,9 +21,12 @@ import org.springframework.web.reactive.function.client.ClientResponse; /** + * Stores the data for a load-balanced call that is being retried. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ -public class LoadBalancerRetryContext { +class LoadBalancerRetryContext { private final ClientRequest request; diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java index 89888b739..0d2edad87 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java @@ -19,39 +19,40 @@ import org.springframework.http.HttpMethod; /** + * Pluggable policy used to establish whether a given load-balanced call should be + * retried. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public interface LoadBalancerRetryPolicy { /** - * Return true to retry the failed request on the same server. This method may be - * called more than once when executing a single operation. - * @param context The context for the retry operation. - * @return True to retry the failed request on the same server; false otherwise. + * Return true to retry on the same service instance. + * @param context the context for the retry operation + * @return true to retry on the same service instance */ boolean canRetrySameServiceInstance(LoadBalancerRetryContext context); /** - * Return true to retry the failed request on the next server from the load balancer. - * This method may be called more than once when executing a single operation. - * @param context The context for the retry operation. - * @return True to retry the failed request on the next server from the load balancer; - * false otherwise. + * Return true to retry on the next service instance. + * @param context the context for the retry operation + * @return true to retry on the same service instance */ boolean canRetryNextServiceInstance(LoadBalancerRetryContext context); /** - * If an exception is not thrown when making a request, this method will be called to - * see if the client would like to retry the request based on the status code - * returned. For example, in Cloud Foundry, the router will return a 404 - * when an app is not available. Since HTTP clients do not throw an exception when a - * 404 is returned, retryableStatusCode allows clients to - * force a retry. - * @param statusCode The HTTP status code. - * @return True if a retry should be attempted; false to just return the response. + * Return true to retry on the provided HTTP status code. + * @param statusCode the HTTP status code + * @return true to retry on the provided HTTP status code */ boolean retryableStatusCode(int statusCode); + /** + * Return true to retry on the provided HTTP method. + * @param method the HTTP request method + * @return true to retry on the provided HTTP method + */ boolean canRetryOnMethod(HttpMethod method); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java index bca6c571e..acb2c95a1 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableExchangeFilterFunctionLoadBalancerRetryPolicy.java @@ -19,7 +19,10 @@ import org.springframework.http.HttpMethod; /** + * The default implementation of {@link LoadBalancerRetryPolicy}. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public class RetryableExchangeFilterFunctionLoadBalancerRetryPolicy implements LoadBalancerRetryPolicy { diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index 769836c34..f28a6e846 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -43,6 +43,7 @@ import org.springframework.http.HttpStatus; import org.springframework.web.reactive.function.client.ClientRequest; import org.springframework.web.reactive.function.client.ClientResponse; +import org.springframework.web.reactive.function.client.ExchangeFilterFunction; import org.springframework.web.reactive.function.client.ExchangeFunction; import static org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools.reconstructURI; @@ -51,17 +52,21 @@ import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.serviceInstanceUnavailableMessage; /** + * An {@link ExchangeFilterFunction} that uses {@link ReactiveLoadBalancer} to execute + * requests against a correct {@link ServiceInstance} and Reactor Retries to retry the + * call both against the same and the next service instance, based on the provided + * {@link LoadBalancerRetryPolicy}. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public class RetryableLoadBalancerExchangeFilterFunction implements LoadBalancedExchangeFilterFunction { - private static final Log LOG = LogFactory - .getLog(RetryableLoadBalancerExchangeFilterFunction.class); + private static final Log LOG = LogFactory.getLog(RetryableLoadBalancerExchangeFilterFunction.class); - private static final List> exceptions = Arrays - .asList(IOException.class, - TimeoutException.class, - org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); + private static final List> exceptions = Arrays.asList(IOException.class, + TimeoutException.class, + org.springframework.cloud.client.loadbalancer.reactive.RetryableStatusCodeException.class); private final LoadBalancerRetryPolicy retryPolicy; diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java index cab2a8ba2..c4814aab3 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableStatusCodeException.java @@ -17,8 +17,12 @@ package org.springframework.cloud.client.loadbalancer.reactive; /** + * An {@link IllegalStateException} used to trigger retries based on the returned HTTP + * status code. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ -public class RetryableStatusCodeException extends IllegalStateException { +class RetryableStatusCodeException extends IllegalStateException { } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java index d5f2b3f6e..36b3678c6 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/DiscoveryClientBasedReactiveLoadBalancer.java @@ -31,7 +31,10 @@ import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; /** + * A {@link ReactiveLoadBalancer} implementation used for tests. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ class DiscoveryClientBasedReactiveLoadBalancer implements ReactiveLoadBalancer { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java index a8abc7e23..31311fb79 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerTestUtils.java @@ -43,7 +43,7 @@ private LoadBalancerTestUtils() { static ConfigurableApplicationContext init(Class... configClasses) { return new SpringApplicationBuilder().web(WebApplicationType.NONE) - .sources(ArrayUtils.add(configClasses, WebClientAutoConfiguration.class)).properties().run(); + .sources(ArrayUtils.add(configClasses, WebClientAutoConfiguration.class)).run(); } @SuppressWarnings("unchecked") diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java index 5267f3648..f03d6e567 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java @@ -57,7 +57,10 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; /** + * Integration tests for {@link RetryableLoadBalancerExchangeFilterFunction}. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ @SpringBootTest(webEnvironment = RANDOM_PORT) class RetryableLoadBalancerExchangeFilterFunctionIntegrationTests { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index 30ff6f7e2..42e18e945 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -38,7 +38,10 @@ import static org.mockito.Mockito.when; /** + * Tests for {@link RetryableLoadBalancerExchangeFilterFunction}. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ @SuppressWarnings("unchecked") class RetryableLoadBalancerExchangeFilterFunctionTests { From f53e8b801b8f4a99658dfc915581a4ba576a76b9 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 16:34:53 +0100 Subject: [PATCH 11/16] Add javadocs. --- docs/src/main/asciidoc/spring-cloud-commons.adoc | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 404fe6e39..a667792c5 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -437,11 +437,13 @@ Then, `ReactiveLoadBalancer` is used underneath. A load-balanced `RestTemplate` can be configured to retry failed requests. By default, this logic is disabled. -You can enable it by adding link:https://github.com/spring-projects/spring-retry[Spring Retry] to your application's classpath. +For the non-reactive version (with `RestTemplate`), you can enable it by adding link:https://github.com/spring-projects/spring-retry[Spring Retry] to your application's classpath. For the reactive version (with `WebTestClient), you need to set `spring.cloud.loadbalancer.retry.enabled=false`. -If you would like to disable the retry logic with Spring Retry on the classpath, you can set `spring.cloud.loadbalancer.retry.enabled=false`. +If you would like to disable the retry logic with Spring Retry or Reactive Retry on the classpath, you can set `spring.cloud.loadbalancer.retry.enabled=false`. -If you would like to implement a `BackOffPolicy` in your retries, you need to create a bean of type `LoadBalancedRetryFactory` and override the `createBackOffPolicy()` method. +For the non-reactive implementation, if you would like to implement a `BackOffPolicy` in your retries, you need to create a bean of type `LoadBalancedRetryFactory` and override the `createBackOffPolicy()` method. + +For the reactive implementation, you just need to enable it by setting `spring.cloud.loadbalancer.retry.backoff.enabled` to `false`. You can set: @@ -449,6 +451,13 @@ 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. +For the reactive implementation, you can additionally set: + - `spring.cloud.loadbalancer.retry.backoff.minBackoff` - sets minimum backoff duration (by default, 5 milliseconds) + - `spring.cloud.loadbalancer.retry.backoff.maxBackoff` - sets maximum backoff duration (by default, max long value of milliseconds) + - `spring.cloud.loadbalancer.retry.backoff.jitter` - sets the jitter used for calculationg the actual backoff duration for each call (by default, 0.5). + +For the reactive implementation, you can also implement your own `LoadBalancerRetryPolicy` to have a more detailed control over the load-balanced call retries. + 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`. ==== From 47c0556e9853b261738d34ea40b9beb9eca96c00 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 16:53:41 +0100 Subject: [PATCH 12/16] Use RetryAwareServiceInstanceListSupplier with reactive retries. --- .../LoadBalancerClientConfiguration.java | 45 +++++++++++++++++-- 1 file changed, 42 insertions(+), 3 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 747b5fecf..dcdc4495d 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,8 @@ package org.springframework.cloud.loadbalancer.annotation; +import reactor.util.retry.RetrySpec; + import org.springframework.boot.autoconfigure.AutoConfigureAfter; import org.springframework.boot.autoconfigure.condition.AllNestedConditions; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; @@ -139,7 +141,7 @@ public ServiceInstanceListSupplier healthCheckDiscoveryClientServiceInstanceList @Configuration(proxyBeanMethods = false) @ConditionalOnBlockingDiscoveryEnabled @ConditionalOnClass(RetryTemplate.class) - @Conditional(OnAvoidPreviousInstanceAndRetryEnabledCondition.class) + @Conditional(BlockingOnAvoidPreviousInstanceAndRetryEnabledCondition.class) @AutoConfigureAfter(BlockingSupportConfiguration.class) @ConditionalOnBean(ServiceInstanceListSupplier.class) public static class BlockingRetryConfiguration { @@ -154,9 +156,27 @@ public ServiceInstanceListSupplier retryAwareDiscoveryClientServiceInstanceListS } - static final class OnAvoidPreviousInstanceAndRetryEnabledCondition extends AllNestedConditions { + @Configuration(proxyBeanMethods = false) + @ConditionalOnBlockingDiscoveryEnabled + @Conditional(ReactiveOnAvoidPreviousInstanceAndRetryEnabledCondition.class) + @AutoConfigureAfter(ReactiveSupportConfiguration.class) + @ConditionalOnBean(ServiceInstanceListSupplier.class) + @ConditionalOnClass(RetrySpec.class) + public static class ReactiveRetryConfiguration { + + @Bean + @ConditionalOnBean(DiscoveryClient.class) + @Primary + public ServiceInstanceListSupplier retryAwareDiscoveryClientServiceInstanceListSupplier( + ServiceInstanceListSupplier delegate) { + return new RetryAwareServiceInstanceListSupplier(delegate); + } + + } + + static final class BlockingOnAvoidPreviousInstanceAndRetryEnabledCondition extends AllNestedConditions { - private OnAvoidPreviousInstanceAndRetryEnabledCondition() { + private BlockingOnAvoidPreviousInstanceAndRetryEnabledCondition() { super(ConfigurationPhase.REGISTER_BEAN); } @@ -174,4 +194,23 @@ static class AvoidPreviousInstanceEnabled { } + static final class ReactiveOnAvoidPreviousInstanceAndRetryEnabledCondition extends AllNestedConditions { + + private ReactiveOnAvoidPreviousInstanceAndRetryEnabledCondition() { + super(ConfigurationPhase.REGISTER_BEAN); + } + + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true") + static class LoadBalancerRetryEnabled { + + } + + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoid-previous-instance", havingValue = "true", + matchIfMissing = true) + static class AvoidPreviousInstanceEnabled { + + } + + } + } From 767132cc303da588306ed00fd732a1ee401e8e77 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 6 Nov 2020 16:57:51 +0100 Subject: [PATCH 13/16] Update properties. --- docs/src/main/asciidoc/_configprops.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/src/main/asciidoc/_configprops.adoc b/docs/src/main/asciidoc/_configprops.adoc index 2bf644f24..98d8e166c 100644 --- a/docs/src/main/asciidoc/_configprops.adoc +++ b/docs/src/main/asciidoc/_configprops.adoc @@ -32,6 +32,10 @@ |spring.cloud.loadbalancer.health-check.path | | |spring.cloud.loadbalancer.hint | | Allows setting the value of hint that is passed on to the LoadBalancer request and can subsequently be used in {@link ReactiveLoadBalancer} implementations. |spring.cloud.loadbalancer.retry.avoid-previous-instance | true | Enables wrapping ServiceInstanceListSupplier beans with `RetryAwareServiceInstanceListSupplier` if Spring-Retry is in the classpath. +|spring.cloud.loadbalancer.retry.backoff.enabled | false | Indicates whether Reactor Retry backoffs should be applied. +|spring.cloud.loadbalancer.retry.backoff.jitter | 0.5 | Used to set {@link RetryBackoffSpec#jitter}. +|spring.cloud.loadbalancer.retry.backoff.max-backoff | | Used to set {@link RetryBackoffSpec#maxBackoff}. +|spring.cloud.loadbalancer.retry.backoff.min-backoff | 5ms | Used to set {@link RetryBackoffSpec#minBackoff}. |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 4e42230a5727ae4a83ba8987f1a15981dd3a796c Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 10 Nov 2020 14:02:44 +0100 Subject: [PATCH 14/16] Fix the docs. --- docs/src/main/asciidoc/spring-cloud-commons.adoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index a667792c5..8916b5a36 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -452,11 +452,11 @@ You can set: - `spring.cloud.loadbalancer.retry.retryableStatusCodes` - the status codes on which to always retry a failed request. For the reactive implementation, you can additionally set: - - `spring.cloud.loadbalancer.retry.backoff.minBackoff` - sets minimum backoff duration (by default, 5 milliseconds) - - `spring.cloud.loadbalancer.retry.backoff.maxBackoff` - sets maximum backoff duration (by default, max long value of milliseconds) - - `spring.cloud.loadbalancer.retry.backoff.jitter` - sets the jitter used for calculationg the actual backoff duration for each call (by default, 0.5). + - `spring.cloud.loadbalancer.retry.backoff.minBackoff` - Sets the minimum backoff duration (by default, 5 milliseconds) + - `spring.cloud.loadbalancer.retry.backoff.maxBackoff` - Sets the maximum backoff duration (by default, max long value of milliseconds) + - `spring.cloud.loadbalancer.retry.backoff.jitter` - Sets the jitter used for calculationg the actual backoff duration for each call (by default, 0.5). -For the reactive implementation, you can also implement your own `LoadBalancerRetryPolicy` to have a more detailed control over the load-balanced call retries. +For the reactive implementation, you can also implement your own `LoadBalancerRetryPolicy` to have more detailed control over the load-balanced call retries. 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`. From 00dea77223c2a7963a2da139e75846f291aa2acc Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 10 Nov 2020 16:54:56 +0100 Subject: [PATCH 15/16] Rename utility class. --- ...rFunctionTools.java => ExchangeFilterFunctionUtils.java} | 4 ++-- .../reactive/ReactorLoadBalancerExchangeFilterFunction.java | 6 +++--- .../RetryableLoadBalancerExchangeFilterFunction.java | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) rename spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/{ExchangeFilterFunctionTools.java => ExchangeFilterFunctionUtils.java} (95%) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionUtils.java similarity index 95% rename from spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java rename to spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionUtils.java index da8fcbe69..18e7124ec 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionTools.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ExchangeFilterFunctionUtils.java @@ -28,9 +28,9 @@ * @author Olga Maciaszek-Sharma * @since 3.0.0 */ -public final class ExchangeFilterFunctionTools { +public final class ExchangeFilterFunctionUtils { - private ExchangeFilterFunctionTools() { + private ExchangeFilterFunctionUtils() { throw new IllegalStateException("Can't instantiate a utility class."); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index 2d53c10dd..6b2c903fa 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -39,9 +39,9 @@ import org.springframework.web.reactive.function.client.ExchangeFunction; import static org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools.reconstructURI; -import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.buildClientRequest; -import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.getHint; -import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.serviceInstanceUnavailableMessage; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionUtils.buildClientRequest; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionUtils.getHint; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionUtils.serviceInstanceUnavailableMessage; /** * An {@link ExchangeFilterFunction} that uses {@link ReactiveLoadBalancer} to execute diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index f28a6e846..8864e9733 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -47,9 +47,9 @@ import org.springframework.web.reactive.function.client.ExchangeFunction; import static org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools.reconstructURI; -import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.buildClientRequest; -import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.getHint; -import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionTools.serviceInstanceUnavailableMessage; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionUtils.buildClientRequest; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionUtils.getHint; +import static org.springframework.cloud.client.loadbalancer.reactive.ExchangeFilterFunctionUtils.serviceInstanceUnavailableMessage; /** * An {@link ExchangeFilterFunction} that uses {@link ReactiveLoadBalancer} to execute From 4cd60f725c0df0294cb9579b1c105cd5de0fd628 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 10 Nov 2020 17:22:14 +0100 Subject: [PATCH 16/16] Verify interactions in order. --- ...adBalancerExchangeFilterFunctionTests.java | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java index 42e18e945..03d87b763 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionTests.java @@ -21,6 +21,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.InOrder; import reactor.core.publisher.Mono; import org.springframework.cloud.client.ServiceInstance; @@ -32,6 +33,7 @@ import org.springframework.web.reactive.function.client.ExchangeFunction; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -62,6 +64,8 @@ class RetryableLoadBalancerExchangeFilterFunctionTests { private final ClientResponse clientResponse = mock(ClientResponse.class); + private final InOrder inOrder = inOrder(next, factory); + @BeforeEach void setUp() { properties.getRetry().setMaxRetriesOnSameServiceInstance(1); @@ -80,8 +84,10 @@ void shouldRetryOnSameAndNextServiceInstanceOnException() { filterFunction.filter(clientRequest, next).subscribe(); - verify(next, times(4)).exchange(any()); - verify(factory, times(2)).getInstance(any()); + inOrder.verify(factory, times(1)).getInstance(any()); + inOrder.verify(next, times(2)).exchange(any()); + inOrder.verify(factory, times(1)).getInstance(any()); + inOrder.verify(next, times(2)).exchange(any()); } @Test @@ -92,8 +98,10 @@ void shouldRetryOnSameAndNextServiceInstanceOnRetryableStatusCode() { filterFunction.filter(clientRequest, next).subscribe(); - verify(next, times(4)).exchange(any()); - verify(factory, times(2)).getInstance(any()); + inOrder.verify(factory, times(1)).getInstance(any()); + inOrder.verify(next, times(2)).exchange(any()); + inOrder.verify(factory, times(1)).getInstance(any()); + inOrder.verify(next, times(2)).exchange(any()); } @Test @@ -135,8 +143,10 @@ void shouldRetryOnMethodOtherThanGetWhenEnabled() { filterFunction.filter(clientRequest, next).subscribe(); - verify(next, times(4)).exchange(any()); - verify(factory, times(2)).getInstance(any()); + inOrder.verify(factory, times(1)).getInstance(any()); + inOrder.verify(next, times(2)).exchange(any()); + inOrder.verify(factory, times(1)).getInstance(any()); + inOrder.verify(next, times(2)).exchange(any()); } }