Update RibbonModule.java #99
Conversation
I noticed several severe issues with the use of feign client and feign.RibbonModule. All this while I was under the impression that it leverages the Ribbon implementation of http client(RestClient) but it does not. I saw the following issues: 1. Connection leaks. The server runs out of file descriptors after a day of hundreds of client calls. 2. Every time a request is made, a new connection is opened. 3. Also some of the Ribbon properties like shiro-client.ribbon.OkToRetryOnAllOperations=false are ignored.
|
feign-pull-requests #132 FAILURE |
|
@amajumdar Can you take a look at the cloudbee build failure? |
|
feign-pull-requests #133 FAILURE |
Minor changes due to changes in super class AbstractLoadBalancerAwareClient
|
feign-pull-requests #134 FAILURE |
|
feign-pull-requests #135 SUCCESS |
There was a problem hiding this comment.
I addressed this compatibility issue in
@adriancole Should we just cherry pick that into master?
There was a problem hiding this comment.
There has been some drift. I think a new commit will need to be made
|
Overall, I think this should work. @adriancole Can you take a look and see if using Ribbon's RestClient will break any Feign's functionality? |
|
Any chance this might get integrated? |
There was a problem hiding this comment.
Is this correct behavior? Eureka source seems to indicate that leaving on the port as a part of the vipAddress is standard:
From AbstractInstanceConfig.java:
@Override
public String getVirtualHostName() {
return (getHostName(false) + ":" + getNonSecurePort());
}Seems to me that honoring exactly what's passed in by the user (minus http/https) is preferable.
|
Let me know if folks still want an opinion on this one. Happy to make a review pass. |
|
this probably needs to be revisited using latest ribbon and latest feign. Feel free to reopen! |
6 participants
I noticed several severe issues with the use of feign client and feign.RibbonModule. All this while I was under the impression that it leverages the Ribbon implementation of http client(RestClient) but it does not.
I saw the following issues: