Extension points for subclassing and some perf optimizations #371
Conversation
…pe of DiscoveryEnabledServer instances created
…rty instead of DynamicIntProperty as a perf optimization
…perf optimization
… they can be overridden in subclasses
…n a new ClientConfigKey so that a different impl can be used
…ies in BaseLoadBalancer
| @@ -198,6 +198,12 @@ public void initWithNiwsConfig(IClientConfig clientConfig) { | |||
| return serverList; | |||
| } | |||
|
|
|||
| protected DiscoveryEnabledServer createServer(final InstanceInfo instanceInfo, boolean useSecurePort, boolean useIpAddr) { | |||
| DiscoveryEnabledServer server = new DiscoveryEnabledServer(instanceInfo, useSecurePort, useIpAddr); | |||
| server.setZone(DiscoveryClient.getZone(instanceInfo)); | |||
There was a problem hiding this comment.
We should improve the API, but for now, can you change from the deprecated DiscoveryClient.getZone to the following?
First, get hold of EurekaClient (there is already an instance), then
EurekaClientConfig clientConfig = eurekaClient.getEurekaClientConfig();
String[] availZones = clientConfig.getAvailabilityZones(clientConfig.getRegion());
String instanceZone = InstanceInfo.getZone(availZones, myInstanceInfo);
server.setZone(instanceZone);
There was a problem hiding this comment.
Absolutely. Although it looks like getEurekaClientConfig() only exists on DiscoveryClient, not EurekaClient - so is it ok for me to cast to DiscoveryClient if it is an instanceof?
There was a problem hiding this comment.
Hmm, looks like ribbon still depends on a really ancient version of eureka, that method was added to the interface from 1.6.0 onwards. Do you mind also updating the eureka dependency to 1.7.2? (1.7.2 is the current latest Java7 compatible version). Thanks.
| } | ||
|
|
||
| void initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping) { | ||
| void initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping, LoadBalancerStats stats) { |
There was a problem hiding this comment.
nit: this is package private so 'should' be safe to do, but just in case since Ribbon is such an old library, can you do an additional overload method to be compatible? Thanks.
void initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping) {
initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping, createLoadBalancerStatsFromConfig(config));
}
| return (LoadBalancerStats) ClientFactory.instantiateInstanceWithClientConfig( | ||
| loadBalancerStatsClassName, clientConfig); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Error initializing LoadBalancerStats", e); |
There was a problem hiding this comment.
instead of throwing a RuntimeException, it may be more compatible if we fallback to emitting a WARN log and create the stats class with the existing new LoadBalancerStats(clientName).
Also: * As had to change the mocking of DiscoveryClient and InstanceInfo in some tests, refactored out that mock code into 2 static methods on LoadBalancerTestUtils. * Upgraded eureka-client dependency version from 1.4.6 to 1.7.2 to get the EurekaClient.getEurekaClientConfig() interface.
|
@qiangdavidliu How does that look? |
|
Looks great. Thanks. |
I've been experimenting with some different load-balancing mechanisms, and had to initially copy-paste some of the ribbon code to allow overriding and extending. Now thats complete, I'd like to get these changes pushed down into ribbon so that I can remove the duplication.
As part of the experimenting, load-testing and profiling, a couple of hotspots came up around the use of
DynamicIntPropertyandjava.util.Random, so I'm submitting those small changes too.Extension points:
LoadBalancerStatsimplementation to be chosen using a newClientConfigKeyinBaseLoadBalancer.LoadBalancerStatsfrom private to protected so they can be overridden in subclasses.DiscoveryEnabledNIWSServerListto override the type ofDiscoveryEnabledServerinstances created by adding a protectedcreateServer()method.Optimizations:
RandomRuleto use aThreadLocalRandominstead ofRandom, as a perf optimization.ServerStatsandLoadBalancerStatsto useCachedDynamicIntPropertyinstead ofDynamicIntPropertyas a perf optimization.