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

Replacement for Ribbon LB #177

Closed
ArtyomGabeev opened this issue Apr 18, 2019 · 8 comments · Fixed by #239
Closed

Replacement for Ribbon LB #177

ArtyomGabeev opened this issue Apr 18, 2019 · 8 comments · Fixed by #239
Assignees
Labels
Projects

Comments

@ArtyomGabeev
Copy link

@ArtyomGabeev ArtyomGabeev commented Apr 18, 2019

Enhancement

As I understand, currently feign supports only Ribbon as client side loadbalancer, which is tight to Eureka.

Since we introduced DiscoveryClient interface, which may be k8s, predefined, etc and have clean interfaces like LoadBalancerClient/ServiceInstanceChooser and ReactiveLoadBalancer,
I think it will be nice to have at least Round Robin LB based on high level DiscoveryClient.

It will allows us to choose between different discovery mechanisms (I'm currently interested in k8s discovery) and provide a basic LB support for feign.

Questions:

  1. is it something already supported?
  2. Is it in cloud openfeign roadmap? If yes or we are interested in it, I may try to contribute.

Thanks

@OlgaMaciaszek

This comment has been minimized.

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek commented Apr 19, 2019

Hi @ArtyomGabeev This is a valid issue and a topic that has yet to be evaluated by the team, probably following a discussion and decisions on Ribbon replacement for the entire Spring Cloud stack (that's the issue to observe on this: spring-cloud/spring-cloud-commons#553 ). Once there's an alternative chosen/ created, we would have to provide it for our OpenFeign integration as well. If you're interested in working on this bit, we will be happy to get the contribution. Will get back to you after we've done some progress on the other issue.

@Glosur

This comment has been minimized.

Copy link

@Glosur Glosur commented Aug 7, 2019

Hey,

Is it something planned ?
I'd like to work on that if needed

@OlgaMaciaszek

This comment has been minimized.

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek commented Aug 16, 2019

Hi @Glosur . yes, we've merged a PR providing our own non-reactive LB client:
spring-cloud/spring-cloud-commons#553 . One of the things I will be doing for Hoxton M3 is switching to it wherever we used Ribbon, but there's a lot of work and help is definitely welcome. Would you like to work on doing that for OpenFeign?

@OlgaMaciaszek OlgaMaciaszek added this to the 2.2.0.M3 milestone Aug 16, 2019
@Glosur

This comment has been minimized.

Copy link

@Glosur Glosur commented Aug 27, 2019

Thanks for the anwser @OlgaMaciaszek

I'd like to give it a try ! Any advice for this ?

@OlgaMaciaszek

This comment has been minimized.

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek commented Sep 12, 2019

Hi @Glosur - as Hoxton is not a major release for SC OpenFeign, it's important to take into account that at this point, we need to start by adding Spring Cloud LoadBalancer to it without removing Ribbon. We have created all the necessary setup to use Spring Cloud LoadBalancer with both non-reactive and reactive setups - I guess for OpenFeign, for now, that would be the non-reactive version.

We have also created spring-cloud-starter-loadbalancer. After including it in your project, you will get a BlockingLoadBalancerClient that is in alternative to the Ribbon client; however, at this point, in order to avoid breaking backward compatibility, we use Ribbon by default. So in order to use the new client, you will have to set spring.cloud.loadbalancer.ribbon.enabled=false.

If you carry this out with a project that previously worked with Ribbon, it will start failing as at various points in Feign Ribbon Client and other Ribbon dependencies are used explicitly; now, an alternative will have to be provided - for example, by switching to using a LoadBalancerClient interface (that would get a different implementation resolved if spring.cloud.loadbalancer.ribbon.enabled=false is set) in place of Ribbon-specific implementation.

I guess there would be more questions, so for further discussion, feel free to contact me at gitter.

@OlgaMaciaszek

This comment has been minimized.

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek commented Sep 27, 2019

Hi @Glosur - please let me know if you are going to work on it. If not, I will get it done soon.

@Glosur

This comment has been minimized.

Copy link

@Glosur Glosur commented Sep 30, 2019

Hello !

I started looking at the code a bit but my computer died yesterday. No problem if you get it done, I'll look at how you will do it

@spencergibb spencergibb added this to To do in Hoxton.RC1 via automation Oct 7, 2019
@spencergibb spencergibb removed this from To do in Hoxton.M3 Oct 7, 2019
@spencergibb spencergibb removed this from the 2.2.0.M3 milestone Oct 7, 2019
@ryanjbaxter ryanjbaxter added this to To do in Hoxton.RC2 via automation Oct 28, 2019
@ryanjbaxter ryanjbaxter removed this from To do in Hoxton.RC1 Oct 28, 2019
@OlgaMaciaszek

This comment has been minimized.

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek commented Oct 29, 2019

Ok. Will work on it now.

@OlgaMaciaszek OlgaMaciaszek self-assigned this Oct 29, 2019
@OlgaMaciaszek OlgaMaciaszek moved this from To do to In progress in Hoxton.RC2 Oct 29, 2019
OlgaMaciaszek added a commit that referenced this issue Nov 4, 2019
httpclient and okhttp setup. Add autoconfiguration. Init fix for gh-177.
Hoxton.RC2 automation moved this from In progress to Done Nov 6, 2019
OlgaMaciaszek added a commit that referenced this issue Nov 6, 2019
* Add FeignBlockingLoadBalancerClient, configurations for default, apache
httpclient and okhttp setup. Add autoconfiguration. Init fix for gh-177.

* Fix SC loadbalancer dependencies.

* Fix generating Feign response when instance not found.

* Fix configuration.

* Add tests for FeignBlockingLoadBalancerClient and FeignLoadBalancerAutoConfiguration.

* Fix checkstyle and formatting.

* Refactor, add docs and javadocs.

* Fix author name.

* Fix docs after code review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Hoxton.RC2
  
Done
5 participants
You can’t perform that action at this time.