Merge pull request #314 from elandau/no_catch_throwable

Remove catch throwables
Netflix · Oct 24, 2016 · 3a707ec · 3a707ec
2 parents 099b5c6 + 9d14a67
commit 3a707ec
Show file tree

Hide file tree

Showing 10 changed files with 130 additions and 145 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
@@ -631,16 +631,16 @@ private VipAddressResolver getVipAddressResolver() {
             synchronized (this) {
                 if (resolver == null) {
                     try {
-                        resolver = (VipAddressResolver) Class.forName(
-                                (String) getProperty(CommonClientConfigKey.VipAddressResolverClassName)).newInstance();
-                    } catch (Throwable 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-evcache/src/main/java/com/netflix/ribbon/evache/EvCacheProvider.java b/ribbon-evcache/src/main/java/com/netflix/ribbon/evache/EvCacheProvider.java
@@ -22,15 +22,16 @@
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import rx.Observable;
-import rx.Observable.OnSubscribe;
-import rx.Subscriber;
 
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 
+import rx.Observable;
+import rx.Observable.OnSubscribe;
+import rx.Subscriber;
+
 /**
  * @author Tomasz Bak
  */
@@ -104,10 +105,9 @@ public void run() {
                     } else if (future.isDone()) {
                         try {
                             handleCompletedFuture(future, subscriber);
-                        } catch (Error e) {
-                            throw e;
-                        } catch (Throwable e) {
-                            LOGGER.warn("unexpected error during checking future result", e);
+                        } catch (InterruptedException e) {
+                            Thread.currentThread().interrupt();
+                            throw new RuntimeException(e);
                         } finally {
                             futureMap.remove(future);
                         }

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,18 +17,19 @@
 */
 package com.netflix.client;
 
-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.
@@ -71,7 +72,7 @@ public class ClientFactory {
     		if (client instanceof AbstractLoadBalancerAwareClient) {
     			((AbstractLoadBalancerAwareClient) client).setLoadBalancer(loadBalancer);
     		}
-    	} catch (Throwable e) {
+    	} catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) {
     		String message = "Unable to InitializeAndAssociateNFLoadBalancer set for RestClient:"
     				+ restClientName;
     		logger.warn(message, e);
@@ -165,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);
@@ -209,7 +209,10 @@ public static Object instantiateInstanceWithClientConfig(String className, IClie
     		    if (clazz.getConstructor(IClientConfig.class) != null) {
     		    	return clazz.getConstructor(IClientConfig.class).newInstance(clientConfig);
     		    }
-    		} catch (Throwable e) { // 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,17 +233,18 @@ public static IClientConfig getNamedConfig(String name) {
      * 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);
+        IClientConfig config = namedConfig.get(name);
         if (config != null) {
             return config;
         } else {
-        	try {
+            try {
                 config = (IClientConfig) clientConfigClass.newInstance();
                 config.loadProperties(name);
-        	} catch (Throwable e) {
-        		logger.error("Unable to create client config instance", e);
-        		return null;
-        	}
+            } 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) {

diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/client/PrimeConnections.java b/ribbon-loadbalancer/src/main/java/com/netflix/client/PrimeConnections.java
@@ -344,10 +344,11 @@ private Boolean connectToServer(final Server s, final PrimeConnectionListener li
         boolean success = false;
         do {
             try {
-                logger.debug("Executing PrimeConnections request to server " + s + " with path " + primeConnectionsURIPath
-                        + ", tryNum=" + tryNum);
+                logger.debug("Executing PrimeConnections request to server {} with path {}, tryNum={}",
+                	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,
@@ -357,20 +358,19 @@ private Boolean connectToServer(final Server s, final PrimeConnectionListener li
                 lastException = e;
                 sleepBeforeRetry(tryNum);
             } 
-            logger.debug("server:" + s + ", result=" + success + ", tryNum="
-                    + tryNum + ", maxRetries=" + maxRetries);
+            logger.debug("server:{}, result={}, tryNum={}, maxRetries={}", s, success, tryNum, maxRetries);
             tryNum++;
         } while (!success && (tryNum <= maxRetries));
         // set the alive flag so that it can be used by load balancers
         if (listener != null) {
             try {
                 listener.primeCompleted(s, lastException);
-            } catch (Throwable e) {
-                logger.error("Error calling PrimeComplete listener", e);
+            } catch (Exception e) {
+                logger.error("Error calling PrimeComplete listener for server '{}'", s, e);
             }
         }
-        logger.debug("Either done, or quitting server:" + s + ", result="
-                + success + ", tryNum=" + tryNum + ", maxRetries=" + maxRetries);        
+        logger.debug("Either done, or quitting server:{}, result={}, tryNum={}, maxRetries={}", 
+        	s, success, tryNum, maxRetries);
         return success;
     }
 

diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/AbstractServerList.java b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/AbstractServerList.java
@@ -49,7 +49,7 @@ public AbstractServerListFilter<T> getFilterImpl(IClientConfig niwsClientConfig)
             AbstractServerListFilter<T> abstractNIWSServerListFilter = 
                     (AbstractServerListFilter<T>) ClientFactory.instantiateInstanceWithClientConfig(niwsServerListFilterClassName, niwsClientConfig);
             return abstractNIWSServerListFilter;
-        } catch (Throwable e) {
+        } catch (InstantiationException | IllegalAccessException | ClassNotFoundException e) {
             throw new ClientException(
                     ClientException.ErrorType.CONFIGURATION,
                     "Unable to get an instance of CommonClientConfigKey.NIWSServerListFilterClassName. Configured class:"