Improvements from PR Netflix#371 review

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.
chengbaoxu · Apr 27, 2018 · 1552fb5 · 1552fb5
1 parent cda93d8
commit 1552fb5
Show file tree

Hide file tree

Showing 10 changed files with 72 additions and 55 deletions.
diff --git a/gradle.properties b/gradle.properties
@@ -4,7 +4,7 @@ servo_version=0.10.1
 hystrix_version=1.4.3
 guava_version=14.0.1
 archaius_version=0.7.6
-eureka_version=1.4.6
+eureka_version=1.7.2
 jersey_version=1.19.1
 
 junit_version=4.12

diff --git a/...on-eureka/src/main/java/com/netflix/niws/loadbalancer/DiscoveryEnabledNIWSServerList.java b/...on-eureka/src/main/java/com/netflix/niws/loadbalancer/DiscoveryEnabledNIWSServerList.java
@@ -27,11 +27,10 @@
 import com.netflix.client.config.IClientConfig;
 import com.netflix.client.config.IClientConfigKey.Keys;
 import com.netflix.config.ConfigurationManager;
-import com.netflix.discovery.DiscoveryClient;
 import com.netflix.discovery.EurekaClient;
+import com.netflix.discovery.EurekaClientConfig;
 import com.netflix.loadbalancer.AbstractServerList;
 import com.netflix.loadbalancer.DynamicServerListLoadBalancer;
-import com.netflix.loadbalancer.LoadBalancerStats;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -200,7 +199,13 @@ private List<DiscoveryEnabledServer> obtainServersViaDiscovery() {
 
     protected DiscoveryEnabledServer createServer(final InstanceInfo instanceInfo, boolean useSecurePort, boolean useIpAddr) {
         DiscoveryEnabledServer server = new DiscoveryEnabledServer(instanceInfo, useSecurePort, useIpAddr);
-        server.setZone(DiscoveryClient.getZone(instanceInfo));
+
+        // Get availabilty zone for this instance.
+        EurekaClientConfig clientConfig = eurekaClientProvider.get().getEurekaClientConfig();
+        String[] availZones = clientConfig.getAvailabilityZones(clientConfig.getRegion());
+        String instanceZone = InstanceInfo.getZone(availZones, instanceInfo);
+        server.setZone(instanceZone);
+
         return server;
     }
 

diff --git a/...ureka/src/test/java/com/netflix/loadbalancer/EurekaDynamicServerListLoadBalancerTest.java b/...ureka/src/test/java/com/netflix/loadbalancer/EurekaDynamicServerListLoadBalancerTest.java
@@ -4,6 +4,7 @@
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.discovery.CacheRefreshedEvent;
+import com.netflix.discovery.DefaultEurekaClientConfig;
 import com.netflix.discovery.DiscoveryClient;
 import com.netflix.discovery.EurekaClient;
 import com.netflix.discovery.EurekaEventListener;
@@ -176,6 +177,8 @@ private boolean verifyFinalServerListCount(int finalCount, DynamicServerListLoad
     private EurekaClient setUpEurekaClientMock(List<InstanceInfo> servers) {
         final EurekaClient eurekaClientMock = PowerMock.createMock(EurekaClient.class);
 
+        EasyMock.expect(eurekaClientMock.getEurekaClientConfig()).andReturn(new DefaultEurekaClientConfig()).anyTimes();
+
         EasyMock
                 .expect(eurekaClientMock.getInstancesByVipAddress(EasyMock.anyString(), EasyMock.anyBoolean(), EasyMock.anyString()))
                 .andReturn(servers.subList(0, initialServerListSize)).times(1)

diff --git a/...a/com/netflix/niws/loadbalancer/DiscoveryEnabledLoadBalancerSupportsPortOverrideTest.java b/...a/com/netflix/niws/loadbalancer/DiscoveryEnabledLoadBalancerSupportsPortOverrideTest.java
@@ -23,7 +23,6 @@
 import com.netflix.config.ConfigurationManager;
 import com.netflix.discovery.DiscoveryClient;
 import com.netflix.discovery.DiscoveryManager;
-import org.easymock.EasyMock;
 
 import org.junit.After;
 import org.junit.Assert;
@@ -35,7 +34,6 @@
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
-import java.util.ArrayList;
 import java.util.List;
 
 import static org.easymock.EasyMock.expect;
@@ -69,21 +67,19 @@ public class DiscoveryEnabledLoadBalancerSupportsPortOverrideTest {
     @Before
     public void setupMock(){
 
-        List<InstanceInfo> dummyII = getDummyInstanceInfo("dummy", "http://www.host.com", "1.1.1.1", 8001);
-        List<InstanceInfo> secureDummyII = getDummyInstanceInfo("secureDummy", "http://www.host.com", "1.1.1.1", 8002);
+        List<InstanceInfo> dummyII = LoadBalancerTestUtils.getDummyInstanceInfo("dummy", "http://www.host.com", "1.1.1.1", 8001);
+        List<InstanceInfo> secureDummyII = LoadBalancerTestUtils.getDummyInstanceInfo("secureDummy", "http://www.host.com", "1.1.1.1", 8002);
 
 
         PowerMock.mockStatic(DiscoveryManager.class);
         PowerMock.mockStatic(DiscoveryClient.class);
 
-        DiscoveryClient mockedDiscoveryClient = createMock(DiscoveryClient.class);
+        DiscoveryClient mockedDiscoveryClient = LoadBalancerTestUtils.mockDiscoveryClient();
         DiscoveryManager mockedDiscoveryManager = createMock(DiscoveryManager.class);
 
-        expect(DiscoveryClient.getZone((InstanceInfo) EasyMock.anyObject())).andReturn("dummyZone").anyTimes();
         expect(DiscoveryManager.getInstance()).andReturn(mockedDiscoveryManager).anyTimes();
         expect(mockedDiscoveryManager.getDiscoveryClient()).andReturn(mockedDiscoveryClient).anyTimes();
 
-
         expect(mockedDiscoveryClient.getInstancesByVipAddress("dummy", false, "region")).andReturn(dummyII).anyTimes();
         expect(mockedDiscoveryClient.getInstancesByVipAddress("secureDummy", true, "region")).andReturn(secureDummyII).anyTimes();
 
@@ -262,23 +258,4 @@ public void testTwoInstancesDontStepOnEachOther() throws Exception{
         Assert.assertEquals(8001, serverList2.get(0).getInstanceInfo().getPort());         // client property indicated in ii
         Assert.assertEquals(7002, serverList2.get(0).getInstanceInfo().getSecurePort());   // 7002 is the secure default
     }
-
-
-
-    protected static List<InstanceInfo> getDummyInstanceInfo(String appName, String host, String ipAddr, int port){
-
-        List<InstanceInfo> list = new ArrayList<InstanceInfo>();
-
-        InstanceInfo info = InstanceInfo.Builder.newBuilder().setAppName(appName)
-                .setHostName(host)
-                .setIPAddr(ipAddr)
-                .setPort(port)
-                .build();
-
-        list.add(info);
-
-        return list;
-
-    }
-
 }
diff --git a/...java/com/netflix/niws/loadbalancer/DiscoveryEnabledLoadBalancerSupportsUseIpAddrTest.java b/...java/com/netflix/niws/loadbalancer/DiscoveryEnabledLoadBalancerSupportsUseIpAddrTest.java
@@ -25,7 +25,6 @@
 import java.util.ArrayList;
 import java.util.List;
 
-import org.easymock.EasyMock;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
@@ -65,22 +64,20 @@ public class DiscoveryEnabledLoadBalancerSupportsUseIpAddrTest {
     @Before
     public void setupMock(){
 
-        List<InstanceInfo> servers = DiscoveryEnabledLoadBalancerSupportsPortOverrideTest.getDummyInstanceInfo("dummy", HOST1, IP1, 8080);
-        List<InstanceInfo> servers2 = DiscoveryEnabledLoadBalancerSupportsPortOverrideTest.getDummyInstanceInfo("dummy", HOST2, IP2, 8080);
+        List<InstanceInfo> servers = LoadBalancerTestUtils.getDummyInstanceInfo("dummy", HOST1, IP1, 8080);
+        List<InstanceInfo> servers2 = LoadBalancerTestUtils.getDummyInstanceInfo("dummy", HOST2, IP2, 8080);
         servers.addAll(servers2);
 
 
         PowerMock.mockStatic(DiscoveryManager.class);
         PowerMock.mockStatic(DiscoveryClient.class);
 
-        DiscoveryClient mockedDiscoveryClient = createMock(DiscoveryClient.class);
+        DiscoveryClient mockedDiscoveryClient = LoadBalancerTestUtils.mockDiscoveryClient();
         DiscoveryManager mockedDiscoveryManager = createMock(DiscoveryManager.class);
 
-        expect(DiscoveryClient.getZone((InstanceInfo) EasyMock.anyObject())).andReturn("dummyZone").anyTimes();
         expect(DiscoveryManager.getInstance()).andReturn(mockedDiscoveryManager).anyTimes();
         expect(mockedDiscoveryManager.getDiscoveryClient()).andReturn(mockedDiscoveryClient).anyTimes();
 
-
         expect(mockedDiscoveryClient.getInstancesByVipAddress("dummy", false, "region")).andReturn(servers).anyTimes();
 
         replay(DiscoveryManager.class);

diff --git a/ribbon-eureka/src/test/java/com/netflix/niws/loadbalancer/LBBuilderTest.java b/ribbon-eureka/src/test/java/com/netflix/niws/loadbalancer/LBBuilderTest.java
@@ -24,7 +24,6 @@
 import com.netflix.loadbalancer.ZoneAffinityServerListFilter;
 import com.netflix.loadbalancer.ZoneAwareLoadBalancer;
 import org.apache.commons.configuration.Configuration;
-import org.easymock.EasyMock;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -33,7 +32,6 @@
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
-import java.util.ArrayList;
 import java.util.List;
 
 import static org.easymock.EasyMock.expect;
@@ -49,16 +47,6 @@ public class LBBuilderTest {
 
     static Server expected = new Server("www.example.com", 8001);
 
-    static List<InstanceInfo> getDummyInstanceInfo(String appName, String host, int port){
-        List<InstanceInfo> list = new ArrayList<InstanceInfo>();
-        InstanceInfo info = InstanceInfo.Builder.newBuilder().setAppName(appName)
-                .setHostName(host)
-                .setPort(port)
-                .build();
-        list.add(info);
-        return list;
-    }
-
     static class NiwsClientConfig extends DefaultClientConfigImpl {
         public NiwsClientConfig() {
             super();
@@ -72,18 +60,16 @@ public String getNameSpace() {
 
     @Before
     public void setupMock(){
-        List<InstanceInfo> instances = getDummyInstanceInfo("dummy", expected.getHost(), expected.getPort());
+        List<InstanceInfo> instances = LoadBalancerTestUtils.getDummyInstanceInfo("dummy", expected.getHost(), "127.0.0.1", expected.getPort());
         PowerMock.mockStatic(DiscoveryManager.class);
         PowerMock.mockStatic(DiscoveryClient.class);
 
-        DiscoveryClient mockedDiscoveryClient = createMock(DiscoveryClient.class);
+        DiscoveryClient mockedDiscoveryClient = LoadBalancerTestUtils.mockDiscoveryClient();
         DiscoveryManager mockedDiscoveryManager = createMock(DiscoveryManager.class);
 
-        expect(DiscoveryClient.getZone((InstanceInfo) EasyMock.anyObject())).andReturn("dummyZone").anyTimes();
         expect(DiscoveryManager.getInstance()).andReturn(mockedDiscoveryManager).anyTimes();
         expect(mockedDiscoveryManager.getDiscoveryClient()).andReturn(mockedDiscoveryClient).anyTimes();
 
-
         expect(mockedDiscoveryClient.getInstancesByVipAddress("dummy:7001", false, null)).andReturn(instances).anyTimes();
 
         replay(DiscoveryManager.class);

diff --git a/ribbon-eureka/src/test/java/com/netflix/niws/loadbalancer/LoadBalancerTestUtils.java b/ribbon-eureka/src/test/java/com/netflix/niws/loadbalancer/LoadBalancerTestUtils.java
@@ -0,0 +1,39 @@
+package com.netflix.niws.loadbalancer;
+
+import com.netflix.appinfo.DataCenterInfo;
+import com.netflix.appinfo.InstanceInfo;
+import com.netflix.appinfo.MyDataCenterInfo;
+import com.netflix.discovery.DefaultEurekaClientConfig;
+import com.netflix.discovery.DiscoveryClient;
+import com.netflix.discovery.DiscoveryManager;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.easymock.EasyMock.expect;
+import static org.powermock.api.easymock.PowerMock.createMock;
+
+public class LoadBalancerTestUtils
+{
+    static List<InstanceInfo> getDummyInstanceInfo(String appName, String host, String ipAddr, int port){
+        List<InstanceInfo> list = new ArrayList<InstanceInfo>();
+
+        InstanceInfo info = InstanceInfo.Builder.newBuilder().setAppName(appName)
+                .setHostName(host)
+                .setIPAddr(ipAddr)
+                .setPort(port)
+                .setDataCenterInfo(new MyDataCenterInfo(DataCenterInfo.Name.MyOwn))
+                .build();
+
+        list.add(info);
+
+        return list;
+    }
+
+    static DiscoveryClient mockDiscoveryClient()
+    {
+        DiscoveryClient mockedDiscoveryClient = createMock(DiscoveryClient.class);
+        expect(mockedDiscoveryClient.getEurekaClientConfig()).andReturn(new DefaultEurekaClientConfig()).anyTimes();
+        return mockedDiscoveryClient;
+    }
+}
diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/BaseLoadBalancer.java b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/BaseLoadBalancer.java
@@ -162,6 +162,10 @@ public BaseLoadBalancer(IClientConfig config) {
     public BaseLoadBalancer(IClientConfig config, IRule rule, IPing ping) {
         initWithConfig(config, rule, ping, createLoadBalancerStatsFromConfig(config));
     }
+
+    void initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping) {
+        initWithConfig(clientConfig, rule, ping, createLoadBalancerStatsFromConfig(config));
+    }
 
     void initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping, LoadBalancerStats stats) {
         this.config = clientConfig;
@@ -232,7 +236,9 @@ private LoadBalancerStats createLoadBalancerStatsFromConfig(IClientConfig client
             return (LoadBalancerStats) ClientFactory.instantiateInstanceWithClientConfig(
                     loadBalancerStatsClassName, clientConfig);
         } catch (Exception e) {
-            throw new RuntimeException("Error initializing LoadBalancerStats", e);
+            logger.warn("Error initializing configured LoadBalancerStats class - " + String.valueOf(loadBalancerStatsClassName)
+                    + ". Falling-back to a new LoadBalancerStats instance instead.", e);
+            return new LoadBalancerStats(clientConfig.getClientName());
         }
     }
 

diff --git a/ribbon-test/build.gradle b/ribbon-test/build.gradle
@@ -3,7 +3,7 @@ dependencies {
     compile project(':ribbon-loadbalancer')
     compile project(':ribbon-eureka')
     compile 'org.codehaus.jackson:jackson-mapper-asl:1.9.11'
-    compile 'com.netflix.eureka:eureka-client:1.1.142'
+    compile "com.netflix.eureka:eureka-client:${eureka_version}"
     compile "io.reactivex:rxjava:${rx_java_version}"
     compile 'javax.ws.rs:jsr311-api:1.1.1'  
     compile "com.google.guava:guava:${guava_version}"

diff --git a/ribbon-test/src/main/java/com/netflix/ribbon/testutils/MockedDiscoveryServerListTest.java b/ribbon-test/src/main/java/com/netflix/ribbon/testutils/MockedDiscoveryServerListTest.java
@@ -15,7 +15,10 @@
  */
 package com.netflix.ribbon.testutils;
 
+import com.netflix.appinfo.DataCenterInfo;
 import com.netflix.appinfo.InstanceInfo;
+import com.netflix.appinfo.MyDataCenterInfo;
+import com.netflix.discovery.DefaultEurekaClientConfig;
 import com.netflix.discovery.DiscoveryClient;
 import com.netflix.discovery.DiscoveryManager;
 import com.netflix.loadbalancer.Server;
@@ -53,6 +56,7 @@ static List<InstanceInfo> getDummyInstanceInfo(String appName, List<Server> serv
             InstanceInfo info = InstanceInfo.Builder.newBuilder().setAppName(appName)
                     .setHostName(server.getHost())
                     .setPort(server.getPort())
+                    .setDataCenterInfo(new MyDataCenterInfo(DataCenterInfo.Name.MyOwn))
                     .build();
             list.add(info);
         }
@@ -68,7 +72,7 @@ public void setupMock(){
         DiscoveryClient mockedDiscoveryClient = createMock(DiscoveryClient.class);
         DiscoveryManager mockedDiscoveryManager = createMock(DiscoveryManager.class);
 
-        expect(DiscoveryClient.getZone((InstanceInfo) EasyMock.anyObject())).andReturn("dummyZone").anyTimes();
+        expect(mockedDiscoveryClient.getEurekaClientConfig()).andReturn(new DefaultEurekaClientConfig()).anyTimes();
         expect(DiscoveryManager.getInstance()).andReturn(mockedDiscoveryManager).anyTimes();
         expect(mockedDiscoveryManager.getDiscoveryClient()).andReturn(mockedDiscoveryClient).anyTimes();