Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could probably update
ChannelToEndpointChannel
to use a caffeine cache with weak keys, and get rid of theisConstant
check altogether once the feign client implementation reuses endpoint values -- worst case is weakref churn and metric fanout, but we don't expect these things to be entirely unbounded. When Endpoints aren't reused, this method is called for every request, so looking up the timers in a large TaggedMetricRegistry can be nontrivial, but code that expects decent performance shouldn't stray far from the standard path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm in favor of this proposal for a couple reasons:
ChannelToEndpointChannel
cache. Using a Caffeine cache would allow to avoid this by use weak keys and/or bounding the cache.ChannelToEndpointChannel
assumes that non-constant endpoints are uniquely identified by(service, endpoint)
, but this isn't correct, for the reasons mentioned in Cache Feign endpoints conjure-java-runtime#2973 (comment). It would probably be good to switch to a cache for this reason alone.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put up #2367. Let me know if this isn't what you were imagining.