Reuse LBClient to avoid duplicate servo registrations#187
Conversation
|
NetflixOSS » feign » feign-pull-requests #55 SUCCESS |
There was a problem hiding this comment.
nit: always linkedhashmap
Discussed in the past, but this has consistent ordering which costs almost nothing. when tests are written and/or JREs switch, this pays dividends.
|
Nice, brendan. I think this is ready for tests. |
|
once this is all tidy, squash and rename the commit to something like.. |
Ribbon does not expect multiple LBClient instances for the same client. This introduces a cache to avoid redundantly creating them. closes OpenFeign#182
bstick12
force-pushed
the
issue-182-noguava
branch
from
ad73b2a to
cd93277
Compare
|
NetflixOSS » feign » feign-pull-requests #57 SUCCESS |
|
@adriancole Changes for #182 and #183 should done now. Could these be merged back to the 7.x? |
|
Yeah. I will cherry-pick and resolve anything that comes up. Thanks!
|
There was a problem hiding this comment.
change from volatile to final. final ensures threads can see this. volatile isn't needed as we aren't changing the reference, rather the contents.
codefromthecrypt
changed the title
|
Pinging spring cloud folks for a second opinion. @philwebb @spencergibb @dsyer So, here's the deal. Brendan hunted through ribbon stack traces and found that each time a LBClient is created, a call to this static beauty ends up happening.
With default config, there's a race possible leading to an exception. The attached code caches the LBClient. Since So, point is this.. do you see any issue merging this code w/notes? Or do you think it is better to punt and say "don't use the default servo registration"? Or do you think there's another solution? |
|
I don't think we use The only alternative approach I can think of would put the cache and locking in Servo, and give monitor clients the option to obtain the current object by name if it already exists. I can see calls to |
|
closing this and will comment in 182 why |
4 participants
issue #182 found that the default configuration of servo does static registration for each LBClient instance. This creates problems at runtime. The solution proposed here is to avoid duplicating LBClient instances by caching them.