spring-cloud / spring-cloud-netflix

Integration with Netflix OSS components
http://cloud.spring.io/spring-cloud-netflix/
Apache License 2.0
4.86k stars 2.44k forks source link

Feign/Ribbon/Eureka - useless RestClient is created for every feign client #312

Closed brenuart closed 9 years ago

brenuart commented 9 years ago

When used together with Ribbon, feign clients delegate the actual call to feign.ribbon.LBClient. In SpringCloud environment, the latter is created by org.springframework.cloud.netflix.feign.ribbon.SpringLBClientFactory that relies on org.springframework.cloud.netflix.ribbon.SpringClientFactory to create a child application context per client.

The SpringClientFactory is responsible to build the child application context hosting all components required by the feign and ribbon components. It also unconditionally adds org.springframework.cloud.netflix.ribbon.RibbonClientConfiguration in the context.

Unfortunately, the latter causes the creation of a RestClient instance never used in this context. RestClient is a pretty heavy component as it comes with an Apache HttpClient and its connection pool behind the scenes... We can see traces of this by looking at the metrics exported by the /metrics actuator endpoint:

counter.servo.<client name>_createnew: 0
counter.servo.<client name>_delete: 0
counter.servo.<client name>_release: 0
counter.servo.<client name>_request: 0
counter.servo.<client name>_reuse: 0

They are created by com.netflix.http4.NamedConnectionPool. Their value stay at 0 whatever the activity.

Proposition is to mark the factory method creating the RestClient as @Lazy in org.springframework.cloud.netflix.ribbon.RibbonClientConfiguration:

@Bean
@Lazy
@ConditionalOnMissingBean
public RestClient ribbonRestClient(IClientConfig config, ILoadBalancer loadBalancer) {
    ...
}

Because of the @Lazy the RestClient will still be created if ever required by any other component.

Side question: is there any other components in that configuration class that should also be considered optional and therefore be annotated with @Lazy?

spencergibb commented 9 years ago

@Lazy could work, but I think it would need to be combined with a change to FeignClientFactoryBean that currently injects the client. It's needed if you don't use ribbon, but it is ignored if you use ribbon.

spencergibb commented 9 years ago

Actually, I was looking at the wrong client in FeignClientFactoryBean, so it may not require the rework there.

brenuart commented 9 years ago

@Lazy on org.springframework.cloud.netflix.ribbon.RibbonClientConfiguration.ribbonRestClient() does the job. I made a checkout of the current sources and applied the change. The RestClient and the underlying Apache HttpClient infrastructure are not created anymore.

spencergibb commented 9 years ago

how about a pull request?

brenuart commented 9 years ago

Ok changes applied. Sorry, but first time I do this with Github - still have to be familiar with this tool ;-) Hope everything is ok. Doesn't build however... Should I configure my Github environment or else?

brenuart commented 9 years ago

Please note that the same could be done to RibbonAutoConfiguration.RibbonClientConfig.restTemplateCustomizer()... What do you think?

brenuart commented 9 years ago

@spencergibb Now that you merged the PR - can we close this issue?