Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support retries for non-loadbalanced requests #532

Open
Ferioney opened this issue Apr 23, 2021 · 5 comments
Open

Support retries for non-loadbalanced requests #532

Ferioney opened this issue Apr 23, 2021 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@Ferioney
Copy link
Contributor

Java 11
Spring Boot 2.3.10.RELEASE
Spring Cloud Hoxton.SR11
Spring Retry 1.2.5.RELEASE

When I create FeignClient with attribute url, load balancer retry doesn't work. In the previous version (Hoxton.SR10) it worked.

Sample: DemoFeignNotRetryApplicationTests

correctRetry test shows that FeignClient without url was retried.
retryDoesNotWork test shows that FeignClient with url wasn't retried.

I think it related to RetryableFeignBlockingLoadBalancerClient does not respect 'url' parameter of @FeignClient.
@OlgaMaciaszek Could you please check?
Thank you!

@OlgaMaciaszek
Copy link
Collaborator

Ther retry support was added for LoadBalancer, hence even all the retry-related properties are prefixed with spring.cloud.loadbalancer.retry. The fact that there were retries when url was passed was sort of an accidental side-effect, they were working cause the requests were being passed via the load-balancing client where they should not have been. We could add retries for non-load-balanced requests as a separate feature, though.

@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request and removed in progress labels Apr 26, 2021
@OlgaMaciaszek OlgaMaciaszek changed the title Hoxton.SR11 LB Retry doesn't work when using Feign Client with url Support retries for non-loadbalanced requests Apr 26, 2021
@OlgaMaciaszek OlgaMaciaszek added this to To do in Hoxton.x via automation Apr 26, 2021
@Aloren
Copy link
Contributor

Aloren commented Apr 26, 2021

I would say that this is an issue of naming things correctly. spring-cloud-loadbalancer states in it's name and properties names that it does only load-balancing and thus we are having problems with supporting some of the features for non loadbalanced requests (configured via url in Feign client). spring.cloud.loadbalancer properties semantically do not fit for non loadbalanced requests, so I guess there might be other issues in the future with the same root cause :(

@OlgaMaciaszek
Copy link
Collaborator

Retries were historically never added as part of the OpenFeign integration. The retry support was introduced in SC Commons as part of the LoadBalancer integrations and added to OpenFeign as part of the effort to support feature parity between the commons LB implementation and the LB implementation used in OpenFeign and that's also where the property comes from. Retries were never supported or requested for non-load-balanced requests in OF. We can introduce this as a new enhancement, though.

@Aloren
Copy link
Contributor

Aloren commented Apr 27, 2021

I think, that I wasn't quite obvious with what I was trying to say.
My idea is that whole chain of feign+loadbalancer is quite strange in terms of retries, but that was before as well with feign+ribbon.
Initially (openfeign + ribbon integration) retries were available both from Feign side via Retryer interface and also via ribbon properties (MaxAutoRetries, MaxAutoRetriesNextServer, OkToRetryOnAllOperations, retryableStatusCodes). It is understandable why those are required, but definitely not user-friendly and error prone to configure retries in different places and making sure that those are in sync. Unfortunately, introducing loadbalancer did not change that concept of retries, instead made it semantically more difficult to understand.

@OlgaMaciaszek
Copy link
Collaborator

For now, we can just add support for non-load-balanced retries (although it is not high-priority, as also Retryer support is present); when we publish next major we can think of improving the API and properties: #535.

@spencergibb spencergibb removed this from To do in Hoxton.x Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants