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.

Sign up for GitHub

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

Jump to bottom

Support lb:// url in FeingClient annotation #489

Open
antechrestos opened this issue Feb 20, 2021 · 8 comments · May be fixed by #518
Open

Support lb:// url in FeingClient annotation #489

antechrestos opened this issue Feb 20, 2021 · 8 comments · May be fixed by #518
Assignees
@OlgaMaciaszek
Labels
waiting for votes

Comments

@antechrestos
Copy link

antechrestos commented Feb 20, 2021 •
edited

Current behaviour

In a project where is used spring cloud gateway, feign client (with target put in configuration), we ended up having feign clients declared as follows

@FeignClient(name="${service.name:any-name}", url="${service.direct-url:}")

With configuration as follows

service:
   name: service-id
   # url: http://direct-url (only used when service discovery is not available)

This way we are able, by configuration, to either run in service discovery/load balancing mode and direct mode.

Proposal

I kind of like the way spring cloud gateway handle that by allowing lb:// protocol. Allowing this protocol in url attribute would allow the following implementation

@FeignClient(name="technical-name", url="${service.uri}")

with following configuration

service:
   uri: "lb://service-id" # or "http//direct-url"

this way it will let common behaviour, allowing to have a common behaviour in url specification.

I spotted the code responsible for feign client building and think that it would be easily doable by

  • only appending http:// if url attribute value does not match the [a-z]+://.* pattern
  • remove load balancing client if and only if url attribute does not starts with lb://

I am eager to implement it if you allow me to do so. I am also pleased to discuss about it and about any alternative I missed.

Thank you
@OlgaMaciaszek OlgaMaciaszek self-assigned this Feb 25, 2021
@antechrestos
Copy link
Author

@OlgaMaciaszek what the point on this issue? Can I make a PR?

antechrestos pushed a commit to antechrestos/spring-cloud-openfeign that referenced this issue Mar 30, 2021
Allow url to use load balance protocol
1595fff 
By specifing 'lb://' protocol, feign client factory will use load
balance client instead of using delegate client

Closes spring-cloud#489
@antechrestos antechrestos linked a pull request Mar 30, 2021 that will close this issue
Open
antechrestos pushed a commit to antechrestos/spring-cloud-openfeign that referenced this issue Mar 30, 2021
Allow url to use load balance protocol
5661a35 
By specifing 'lb://' protocol, feign client factory will use load
balance client instead of using delegate client

Closes spring-cloud#489
@antechrestos
Copy link
Author

PR submitted

@OlgaMaciaszek
Copy link
Collaborator

@antechrestos Actually, I am not sure this is a change we want to make. We are going to see if there's interest in the community and, if yes, then, we will discuss it in the team.

@antechrestos
Copy link
Author

@OlgaMaciaszek Agreed. Just for clarification, is there any documentation describing "the waiting for votes" adhesion and what is the comunity? Is there a number of 👍 expected on the issue? Or is it based on an adhesion of an inner community (spring developers for instance)?
Thank you

@OlgaMaciaszek
Copy link
Collaborator

We check how many users request a feature. If we see considerable interest, then we discuss the possible issues and implications within the Spring Cloud team, taking into account the discussion in the issue and other considerations related to maintenance, planned future project development and possible issues.

@antechrestos
Copy link
Author

@OlgaMaciaszek ok that's more clear

@antechrestos
Copy link
Author

To my mind, I think the real question is if there is value to apply the concept of lb:// url used in spring cloud gateway in other spring projects or if it needs to be restricted to it.

As an alternative, it may be useful to let the user decide whether (s)he wants to skip load balancing/discovery behaviour. Because currently, the url use hides the fact that we force the use of delegated client... What do you think of it?

@antechrestos
Copy link
Author

@OlgaMaciaszek do you think I should open a new issue proposing to handle load-balancing/discovery in a dedicated properties rather than hidden behind url attribute?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@OlgaMaciaszek OlgaMaciaszek

Labels
waiting for votes
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow load balancer uri in uri attribute antechrestos/spring-cloud-openfeign
3 participants
@OlgaMaciaszek @antechrestos @spring-cloud-issues