From 23ca17df2c27f8eea17cf5968bb8c62111a427eb Mon Sep 17 00:00:00 2001 From: apikozh Date: Fri, 3 Mar 2023 12:54:13 +0100 Subject: [PATCH] Reuse serviceInstance for retries Fixes gh-840 (#841) --- ...ryableFeignBlockingLoadBalancerClient.java | 7 +-- ...eFeignBlockingLoadBalancerClientTests.java | 44 ++++++++++++++++++- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClient.java index 2747b29a..d020874e 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClient.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2022 the original author or authors. + * Copyright 2013-2023 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. @@ -66,6 +66,7 @@ import static org.springframework.cloud.openfeign.loadbalancer.LoadBalancerUtils * * @author Olga Maciaszek-Sharma * @author Wonsik Cheung + * @author Andriy Pikozh * @since 2.2.6 */ @SuppressWarnings({ "rawtypes", "unchecked" }) @@ -125,8 +126,8 @@ public class RetryableFeignBlockingLoadBalancerClient implements Client { // and extract the server and update the request being made if (context instanceof LoadBalancedRetryContext) { LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; - ServiceInstance serviceInstance = lbContext.getServiceInstance(); - if (serviceInstance == null) { + retrievedServiceInstance = lbContext.getServiceInstance(); + if (retrievedServiceInstance == null) { if (LOG.isDebugEnabled()) { LOG.debug("Service instance retrieved from LoadBalancedRetryContext: was null. " + "Reattempting service instance selection"); diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClientTests.java index 84310745..036b4674 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClientTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableFeignBlockingLoadBalancerClientTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2022 the original author or authors. + * Copyright 2013-2023 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. @@ -74,6 +74,7 @@ import static org.mockito.Mockito.when; * "https://github.com/spring-cloud/spring-cloud-commons/blob/main/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java">BlockingLoadBalancerClientTests * @author Olga Maciaszek-Sharma * @author Wonsik Cheung + * @author Andriy Pikozh */ @ExtendWith(MockitoExtension.class) class RetryableFeignBlockingLoadBalancerClientTests { @@ -158,10 +159,51 @@ class RetryableFeignBlockingLoadBalancerClientTests { feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + verify(loadBalancerClient, times(2)).choose(eq("test"), any()); verify(loadBalancerClient, times(2)).reconstructURI(serviceInstance, URI.create("http://test/path")); verify(delegate, times(2)).execute(any(), any()); } + @Test + void shouldReuseServerInstanceOnSameInstanceRetry() throws IOException { + properties.getRetry().setMaxRetriesOnSameServiceInstance(1); + properties.getRetry().setMaxRetriesOnNextServiceInstance(0); + properties.getRetry().getRetryableStatusCodes().add(503); + Request request = testRequest(); + Response response = testResponse(503); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + when(loadBalancerClient.reconstructURI(serviceInstance, URI.create("http://test/path"))) + .thenReturn(URI.create("http://testhost:80/path")); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(loadBalancerClient, times(1)).choose(eq("test"), any()); + verify(loadBalancerClient, times(2)).reconstructURI(serviceInstance, URI.create("http://test/path")); + verify(delegate, times(2)).execute(any(), any()); + } + + @Test + void shouldReuseServerInstanceOnSameInstanceRetryWithBothSameAndNextRetries() throws IOException { + properties.getRetry().setMaxRetriesOnSameServiceInstance(1); + properties.getRetry().setMaxRetriesOnNextServiceInstance(1); + properties.getRetry().getRetryableStatusCodes().add(503); + Request request = testRequest(); + Response response = testResponse(503); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + when(loadBalancerClient.reconstructURI(serviceInstance, URI.create("http://test/path"))) + .thenReturn(URI.create("http://testhost:80/path")); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(loadBalancerClient, times(2)).choose(eq("test"), any()); + verify(loadBalancerClient, times(4)).reconstructURI(serviceInstance, URI.create("http://test/path")); + verify(delegate, times(4)).execute(any(), any()); + } + @Test void shouldNotRetryOnDisabled() throws IOException { properties.getRetry().setEnabled(false);