Issue 182#186
Conversation
|
NetflixOSS » feign » feign-pull-requests #54 SUCCESS |
There was a problem hiding this comment.
guava code makes me nervous (eventhough ribbon depends on it). Also static makes me nervous.
if ribbon wants to coordinate everything in the VM, we could propagate that advice by saying "use a single instance of RibbonClient() for all feigns in the VM". This removes us from having to do so explicitly by making a static cache.
Next, we could move load into a synchronized method getOrCreateLBClient that does a guarded check on an internal linkedhashmap cache. if the key is not present, populate it then return the value.
I'd be surprised if we would need to optimize getOrCreateLBClient to be something besides just synchronized.
Make sense?
There was a problem hiding this comment.
No problem with the changes suggested but a couple of counterpoints
- Using a single RibbonClient across the entire VM might be a bit limiting as this forces the use of only a single Client delegate. Which I believe is probably fine if you are using the Default.Client. But maybe not possible with a custom Client delegate.
- Not using a static map will mean that the error will still occur if someone disregards the advice or needs to use different delegates - This would be my main concern.
- If we create a synchronized method getOrCreateLBClient this means that all request via the RibbonClient will incur the penalty ( we can avoid this with some double locking and volatile)
|
@bstick12 I agree that there are some tradeoffs, and also, I think some of this is reacting to design tradeoffs in Ribbon. For example, you could imagine that if such safety is required to not create dupes, that could be addressed upstream. WRT lock contention, if profiling shows that the simple solution is a limiting factor (as opposed to other code in feign or ribbon), I agree we could optimize it. I totally understand the concern about safety with ribbon and that reading javadoc here punts that. Not to repeat, but I think this is us reacting to a problem that could be solved upstream, hopefully making this resolution tentative. @allenxwang any ideas on how ribbon consumers can be less concerned wrt JVM singletons? |
|
@adriancole There looks to be a possible work around by creating a NO-OP implementation of the com.netflix.servo.MonitorRegistry which would have to be configured via the system property com.netflix.servo.DefaultMonitorRegistry.registryClass |
|
Duplicated by #187 |
|
With that configured, we wouldn't need this change? If so, I'd rather us
doc than merge. Less is best.
|
|
Ps even though I am not a fan of making the cache static, I think it is
better than exposing RibbonClient.INSTANCE. if code is still needed (ie
cannot be solved with configuration), go ahead and switch to a static final
map (and lock on it).
|
|
If we just document a fix to the issue then we will remove the warning message that is being logged. We would still be needlessly creating an LBClient for each request. I can't comment on what the overhead of this is. If we are going to document should be we providing an NoOpMonitorRegistry class? |
|
opened a fix upstream. Netflix/servo#306 |
3 participants
Not sure how you feel about using a LoadingCache but this will solve the JMX registration race condition