Use Spring Retry For Failed Requests Using Feign #1457
Conversation
|
This is not likely ready for prime time but I wanted to get some eyes on these changes. |
|
spring-cloud/spring-cloud-sleuth#446 is dependent on this change |
|
One last comment ;) These changes will also need to be merged into the 1.3.x branch. |
Current coverage is 74.36% (diff: 76.56%)@@ 1.2.x #1457 diff @@
==========================================
Files 189 190 +1
Lines 5834 5886 +52
Methods 0 0
Messages 0 0
Branches 884 888 +4
==========================================
+ Hits 4350 4377 +27
- Misses 1166 1191 +25
Partials 318 318
|
| @@ -16,6 +16,13 @@ | |||
|
|
|||
| package org.springframework.cloud.netflix.feign; | |||
|
|
|||
| import com.netflix.hystrix.HystrixCommand; | |||
There was a problem hiding this comment.
Seems odd. Why a bunch of new imports?
| Response response = request.client().execute(request.toRequest(), options); | ||
| return new RibbonResponse(request.getUri(), response); | ||
| LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory.create(this.getClientName(), this); | ||
| retryTemplate.setRetryPolicy(retryPolicy == null ? new NeverRetryPolicy() |
There was a problem hiding this comment.
Seems like this should only be done once.
There was a problem hiding this comment.
We need the request when creating the policy
There was a problem hiding this comment.
I'm still confused about where the request is used.
There was a problem hiding this comment.
So you are saying that the policy should only be created once? The problem is we need the Request when FeignRetryPolicy.open is called so we can set the ServiceInstance in the Retry Context.
There was a problem hiding this comment.
I see, the retry policy created below uses the request. Could there be a problem since the retry policy is scoped to the request and the template is a field?
There was a problem hiding this comment.
Just trying to make sure I understand the concern....
If multiple requests are executed simultaneously you are concerned about switching out the retry policy mid request?
There was a problem hiding this comment.
I agree, I guess the only good solution for now is to create a new RetryTemplate for each request. I don't see any real downside to that, but I do think it can be handled better by Spring Retry. I posed this in issue spring-projects/spring-retry#71.
| @@ -71,7 +68,7 @@ | |||
| * Flag for whether retry is supported by default (assuming the routes themselves | |||
| * support it). | |||
| */ | |||
| private Boolean retryable; | |||
| private Boolean retryable = false; | |||
|
|
||
| private volatile Map<String, FeignLoadBalancer> cache = new ConcurrentReferenceHashMap<>(); | ||
|
|
||
| public CachingSpringLoadBalancerFactory(SpringClientFactory factory) { | ||
| public CachingSpringLoadBalancerFactory(SpringClientFactory factory, RetryTemplate retryTemplate, |
There was a problem hiding this comment.
We should keep the old signature for backwards compatibility.
| @@ -16,8 +16,12 @@ | |||
|
|
|||
| package org.springframework.cloud.netflix.ribbon.apache; | |||
|
|
|||
| import java.net.URI; | |||
|
|
|||
| import com.netflix.client.RequestSpecificRetryHandler; | |||
There was a problem hiding this comment.
You need to fix your ordering in a number of files ;-)
…s per request -Fixed imports -Addressed other code review concerns
|
@spencergibb can you take another look when you get a chance? |
| @@ -107,7 +106,7 @@ public FormattingConversionService feignConversionService() { | |||
| @Scope("prototype") | |||
| @ConditionalOnMissingBean | |||
| public Feign.Builder feignBuilder() { | |||
| return Feign.builder(); | |||
| return Feign.builder().retryer(Retryer.NEVER_RETRY); | |||
There was a problem hiding this comment.
This should go in a @Bean definition.
| Response response = request.client().execute(request.toRequest(), options); | ||
| return new RibbonResponse(request.getUri(), response); | ||
| LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory.create(this.getClientName(), this); | ||
| retryTemplate.setRetryPolicy(retryPolicy == null ? new NeverRetryPolicy() |
There was a problem hiding this comment.
I'm still confused about where the request is used.
… dependent on the request. We don’t want two Feign requests stomping on each other by switching out the retry policy for the template so each request now created a new template to use.
Part of our continued effort to unify retry logic in Spring Cloud around Spring Retry. Ties back to #1290.