address PR comments

Netflix · Oct 21, 2016 · 9d14a67 · 9d14a67
1 parent c241075
commit 9d14a67
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 59 deletions.
diff --git a/ribbon-core/src/main/java/com/netflix/client/config/DefaultClientConfigImpl.java b/ribbon-core/src/main/java/com/netflix/client/config/DefaultClientConfigImpl.java
@@ -17,24 +17,24 @@
 */
 package com.netflix.client.config;
 
-import java.util.Iterator;
-import java.util.Map;
-import java.util.Properties;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.TimeUnit;
-
-import org.apache.commons.configuration.AbstractConfiguration;
-import org.apache.commons.configuration.Configuration;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import com.google.common.base.Strings;
 import com.netflix.client.VipAddressResolver;
 import com.netflix.config.ConfigurationManager;
 import com.netflix.config.DynamicProperty;
 import com.netflix.config.DynamicPropertyFactory;
 import com.netflix.config.DynamicStringProperty;
 
+import org.apache.commons.configuration.AbstractConfiguration;
+import org.apache.commons.configuration.Configuration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
 /**
  * Default client configuration that loads properties from Archaius's ConfigurationManager.
  * <p>
@@ -631,16 +631,16 @@ private VipAddressResolver getVipAddressResolver() {
             synchronized (this) {
                 if (resolver == null) {
                     try {
-                        resolver = (VipAddressResolver) Class.forName(
-                                (String) getProperty(CommonClientConfigKey.VipAddressResolverClassName)).newInstance();
-                    } catch (InstantiationException|IllegalAccessException|ClassNotFoundException e) {
-                        LOG.error("Cannot instantiate VipAddressResolver", e);
-		    }
+                        resolver = (VipAddressResolver) Class
+                                .forName((String) getProperty(CommonClientConfigKey.VipAddressResolverClassName))
+                                .newInstance();
+                    } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) {
+                        throw new RuntimeException("Cannot instantiate VipAddressResolver", e);
+                    }
                 }
             }
         }
         return resolver;
-
     }
 
     @Override

diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/client/ClientFactory.java b/ribbon-loadbalancer/src/main/java/com/netflix/client/ClientFactory.java
@@ -17,19 +17,19 @@
 */
 package com.netflix.client;
 
-import java.lang.reflect.InvocationTargetException;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.loadbalancer.ILoadBalancer;
 import com.netflix.servo.monitor.Monitors;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
 /**
  * A factory that creates client, load balancer and client configuration instances from properties. It also keeps mappings of client names to 
  * the  created instances.
@@ -166,8 +166,7 @@ public static ILoadBalancer registerNamedLoadBalancerFromclientConfig(String nam
             String loadBalancerClassName = (String) clientConfig.getProperty(CommonClientConfigKey.NFLoadBalancerClassName);
             lb = (ILoadBalancer) ClientFactory.instantiateInstanceWithClientConfig(loadBalancerClassName, clientConfig);                                    
             namedLBMap.put(name, lb);            
-            logger.info("Client:" + name
-                    + " instantiated a LoadBalancer:" + lb.toString());
+            logger.info("Client: {} instantiated a LoadBalancer: {}", name, lb);
             return lb;
         } catch (Exception e) {           
            throw new ClientException("Unable to instantiate/associate LoadBalancer with Client:" + name, e);
@@ -210,7 +209,10 @@ public static Object instantiateInstanceWithClientConfig(String className, IClie
     		    if (clazz.getConstructor(IClientConfig.class) != null) {
     		    	return clazz.getConstructor(IClientConfig.class).newInstance(clientConfig);
     		    }
-    		} catch (NoSuchMethodException | SecurityException | IllegalArgumentException | InvocationTargetException ignore) { // NOPMD   			
+    		} catch (NoSuchMethodException ignored) {
+    		    // OK for a class to not take an IClientConfig
+    		} catch (SecurityException | IllegalArgumentException | InvocationTargetException e) { 
+    		    logger.warn("Error getting/invoking IClientConfig constructor of {}", className, e);
     		}    		
     	}
     	logger.warn("Class " + className + " neither implements IClientConfigAware nor provides a constructor with IClientConfig as the parameter. Only default constructor will be used.");
@@ -230,25 +232,25 @@ public static IClientConfig getNamedConfig(String name) {
      * Get the client configuration given the name or create one with clientConfigClass if it does not exist. An instance of IClientConfig
      * is created and {@link IClientConfig#loadProperties(String)} will be called.
      */
-	public static IClientConfig getNamedConfig(String name, Class<? extends IClientConfig> clientConfigClass) {
-		IClientConfig config = namedConfig.get(name);
-		if (config != null) {
-			return config;
-		} else {
-			try {
-				config = (IClientConfig) clientConfigClass.newInstance();
-				config.loadProperties(name);
-			} catch (InstantiationException | IllegalAccessException e) {
-				logger.error("Unable to create named client config '{}' instance for config class {}", name,
-				        clientConfigClass, e);
-				return null;
-			}
-			config.loadProperties(name);
-			IClientConfig old = namedConfig.putIfAbsent(name, config);
-			if (old != null) {
-				config = old;
-			}
-			return config;
-		}
-	}
+    public static IClientConfig getNamedConfig(String name, Class<? extends IClientConfig> clientConfigClass) {
+        IClientConfig config = namedConfig.get(name);
+        if (config != null) {
+            return config;
+        } else {
+            try {
+                config = (IClientConfig) clientConfigClass.newInstance();
+                config.loadProperties(name);
+            } catch (InstantiationException | IllegalAccessException e) {
+                logger.error("Unable to create named client config '{}' instance for config class {}", name,
+                        clientConfigClass, e);
+                return null;
+            }
+            config.loadProperties(name);
+            IClientConfig old = namedConfig.putIfAbsent(name, config);
+            if (old != null) {
+                config = old;
+            }
+            return config;
+        }
+    }
 }
diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/client/PrimeConnections.java b/ribbon-loadbalancer/src/main/java/com/netflix/client/PrimeConnections.java
@@ -348,6 +348,7 @@ private Boolean connectToServer(final Server s, final PrimeConnectionListener li
                 	s, primeConnectionsURIPath, tryNum);
                 success = connector.connect(s, primeConnectionsURIPath);
                 successCounter.increment();
+                lastException = null;
                 break;
             } catch (Exception e) {
                 // It does not really matter if there was an exception,
@@ -365,7 +366,7 @@ private Boolean connectToServer(final Server s, final PrimeConnectionListener li
             try {
                 listener.primeCompleted(s, lastException);
             } catch (Exception e) {
-                logger.error("Error calling PrimeComplete listener for server '{}'", s.getHost(), e);
+                logger.error("Error calling PrimeComplete listener for server '{}'", s, e);
             }
         }
         logger.debug("Either done, or quitting server:{}, result={}, tryNum={}, maxRetries={}", 

diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/BaseLoadBalancer.java b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/BaseLoadBalancer.java
@@ -189,7 +189,7 @@ void initWithConfig(IClientConfig clientConfig, IRule rule, IPing ping) {
         if (ping instanceof AbstractLoadBalancerPing) {
             ((AbstractLoadBalancerPing) ping).setLoadBalancer(this);
         }
-        logger.info("Client: {} instantiated a LoadBalancer: {}", name, toString());
+        logger.info("Client: {} instantiated a LoadBalancer: {}", name, this);
         boolean enablePrimeConnections = clientConfig.get(
                 CommonClientConfigKey.EnablePrimeConnections, DefaultClientConfigImpl.DEFAULT_ENABLE_PRIME_CONNECTIONS);
 
@@ -645,12 +645,11 @@ public Pinger(IPingStrategy pingerStrategy) {
             this.pingerStrategy = pingerStrategy;
         }
 
-        public void runPinger() {
-            if (pingInProgress.get()) {
+        public void runPinger() throws Exception {
+            if (!pingInProgress.compareAndSet(false, true)) { 
                 return; // Ping in progress - nothing to do
-            } else {
-                pingInProgress.set(true);
             }
+
             // we are "in" - we get to Ping
 
             Server[] allServers = null;
@@ -698,9 +697,6 @@ public void runPinger() {
                 upLock.unlock();
 
                 notifyServerStatusChangeListener(changedServers);
-
-            } catch (Exception e) {
-                logger.error("LoadBalancer [{}] : Error running the Pinger", name, e);
             } finally {
                 pingInProgress.set(false);
             }
@@ -753,8 +749,7 @@ public String choose(Object key) {
                 Server svr = rule.choose(key);
                 return ((svr == null) ? null : svr.getId());
             } catch (Exception e) {
-            	logger.error("LoadBalancer [{}]:  Error choosing server for key '{}'", name, key, e);
-                return null;
+                throw new RuntimeException("Error choosing server", e);
             }
         }
     }