SOLR-5934: LBHttpSolrServer exception handling improvement and small test improvements.

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1583213 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -250,6 +250,9 @@ Other Changes
 * SOLR-5914: Cleanup and fix Solr's test cleanup code. 
  (Mark Miller, Uwe Schindler)
 
+* SOLR-5934: LBHttpSolrServer exception handling improvement and small test
+  improvements. (Gregory Chanan via Mark Miller)
+
 ==================  4.7.1  ==================
 
 Versions of Major Components

solr/core/src/test/org/apache/solr/cloud/ChaosMonkeyNothingIsSafeTest.java

@@ -75,14 +75,14 @@ public class ChaosMonkeyNothingIsSafeTest extends AbstractFullDistribZkTestBase
     SolrCmdDistributor.testing_errorHook = null;
   }
   
-  public static String[] fieldNames = new String[]{"f_i", "f_f", "f_d", "f_l", "f_dt"};
+  protected static final String[] fieldNames = new String[]{"f_i", "f_f", "f_d", "f_l", "f_dt"};
-  public static RandVal[] randVals = new RandVal[]{rint, rfloat, rdouble, rlong, rdate};
+  protected static final RandVal[] randVals = new RandVal[]{rint, rfloat, rdouble, rlong, rdate};
   
-  protected String[] getFieldNames() {
+  public String[] getFieldNames() {
     return fieldNames;
   }
 
-  protected RandVal[] getRandValues() {
+  public RandVal[] getRandValues() {
     return randVals;
   }
   

solr/core/src/test/org/apache/solr/cloud/ChaosMonkeySafeLeaderTest.java

@@ -58,14 +58,14 @@ public class ChaosMonkeySafeLeaderTest extends AbstractFullDistribZkTestBase {
     SolrCmdDistributor.testing_errorHook = null;
   }
   
-  public static String[] fieldNames = new String[]{"f_i", "f_f", "f_d", "f_l", "f_dt"};
+  protected static final String[] fieldNames = new String[]{"f_i", "f_f", "f_d", "f_l", "f_dt"};
-  public static RandVal[] randVals = new RandVal[]{rint, rfloat, rdouble, rlong, rdate};
+  protected static final RandVal[] randVals = new RandVal[]{rint, rfloat, rdouble, rlong, rdate};
   
-  protected String[] getFieldNames() {
+  public String[] getFieldNames() {
     return fieldNames;
   }
 
-  protected RandVal[] getRandValues() {
+  public RandVal[] getRandValues() {
     return randVals;
   }
   

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrServer.java

@@ -298,85 +298,17 @@ public class LBHttpSolrServer extends SolrServer {
       rsp.server = serverStr;
       HttpSolrServer server = makeServer(serverStr);
 
-      try {
+      ex = doRequest(server, req, rsp, isUpdate, false, null);
-        rsp.rsp = server.request(req.getRequest());
+      if (ex == null) {
         return rsp; // SUCCESS
-      } catch (SolrException e) {
-        // we retry on 404 or 403 or 503 or 500
-        // unless it's an update - then we only retry on connect exceptions
-        if (!isUpdate && RETRY_CODES.contains(e.code())) {
-          ex = addZombie(server, e);
-        } else {
-          // Server is alive but the request was likely malformed or invalid
-          throw e;
-        }
-      } catch (SocketException e) {
-        if (!isUpdate || e instanceof ConnectException) {
-          ex = addZombie(server, e);
-        } else {
-          throw e;
-        }
-      } catch (SocketTimeoutException e) {
-        if (!isUpdate) {
-          ex = addZombie(server, e);
-        } else {
-          throw e;
-        }
-      } catch (SolrServerException e) {
-        Throwable rootCause = e.getRootCause();
-        if (!isUpdate && rootCause instanceof IOException) {
-          ex = addZombie(server, e);
-        } else if (isUpdate && rootCause instanceof ConnectException) {
-          ex = addZombie(server, e);
-        } else {
-          throw e;
-        }
-      } catch (Exception e) {
-        throw new SolrServerException(e);
       }
     }
 
     // try the servers we previously skipped
     for (ServerWrapper wrapper : skipped) {
-      try {
+      ex = doRequest(wrapper.solrServer, req, rsp, isUpdate, true, wrapper.getKey());
-        rsp.rsp = wrapper.solrServer.request(req.getRequest());
+      if (ex == null) {
-        zombieServers.remove(wrapper.getKey());
+         return rsp; // SUCCESS
-        return rsp; // SUCCESS
-      } catch (SolrException e) {
-        // we retry on 404 or 403 or 503 or 500
-        // unless it's an update - then we only retry on connect exceptions
-        if (!isUpdate && RETRY_CODES.contains(e.code())) {
-          ex = e;
-          // already a zombie, no need to re-add
-        } else {
-          // Server is alive but the request was malformed or invalid
-          zombieServers.remove(wrapper.getKey());
-          throw e;
-        }
-
-      } catch (SocketException e) {
-        if (!isUpdate || e instanceof ConnectException) {
-          ex = e;
-        } else {
-          throw e;
-        }
-      } catch (SocketTimeoutException e) {
-        if (!isUpdate) {
-          ex = e;
-        } else {
-          throw e;
-        }
-      } catch (SolrServerException e) {
-        Throwable rootCause = e.getRootCause();
-        if (!isUpdate && rootCause instanceof IOException) {
-          ex = e;
-        } else if (isUpdate && rootCause instanceof ConnectException) {
-          ex = e;
-        } else {
-          throw e;
-        }
-      } catch (Exception e) {
-        throw new SolrServerException(e);
       }
     }
 
@@ -401,7 +333,53 @@ public class LBHttpSolrServer extends SolrServer {
     return e;
   }  
 
+  protected Exception doRequest(HttpSolrServer server, Req req, Rsp rsp, boolean isUpdate,
+      boolean isZombie, String zombieKey) throws SolrServerException, IOException {
+    Exception ex = null;
+    try {
+      rsp.rsp = server.request(req.getRequest());
+      if (isZombie) {
+        zombieServers.remove(zombieKey);
+      }
+    } catch (SolrException e) {
+      // we retry on 404 or 403 or 503 or 500
+      // unless it's an update - then we only retry on connect exception
+      if (!isUpdate && RETRY_CODES.contains(e.code())) {
+        ex = (!isZombie) ? addZombie(server, e) : e;
+      } else {
+        // Server is alive but the request was likely malformed or invalid
+        if (isZombie) {
+          zombieServers.remove(zombieKey);
+        }
+        throw e;
+      }
+    } catch (SocketException e) {
+      if (!isUpdate || e instanceof ConnectException) {
+        ex = (!isZombie) ? addZombie(server, e) : e;
+      } else {
+        throw e;
+      }
+    } catch (SocketTimeoutException e) {
+      if (!isUpdate) {
+        ex = (!isZombie) ? addZombie(server, e) : e;
+      } else {
+        throw e;
+      }
+    } catch (SolrServerException e) {
+      Throwable rootCause = e.getRootCause();
+      if (!isUpdate && rootCause instanceof IOException) {
+        ex = (!isZombie) ? addZombie(server, e) : e;
+      } else if (isUpdate && rootCause instanceof ConnectException) {
+        ex = (!isZombie) ? addZombie(server, e) : e;
+      } else {
+        throw e;
+      }
+    } catch (Exception e) {
+      throw new SolrServerException(e);
+    }
 
+    return ex;
+  }
 
   private void updateAliveList() {
     synchronized (aliveServers) {

solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java

@@ -1137,27 +1137,23 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes
     Set<SolrDocument> onlyInB = new HashSet<>(setB);
     onlyInB.removeAll(setA);
 
-    if (onlyInA.size() > 0) {
+    for (SolrDocument doc : onlyInA) {
-      for (SolrDocument doc : onlyInA) {
+      if (!addFails.contains(doc.getFirstValue("id"))) {
-        if (!addFails.contains(doc.getFirstValue("id"))) {
+        legal = false;
-          legal = false;
+      } else {
-        } else {
+        System.err.println("###### Only in " + aName + ": " + onlyInA
-          System.err.println("###### Only in " + aName + ": " + onlyInA
+            + ", but this is expected because we found an add fail for "
-              + ", but this is expected because we found an add fail for "
+            + doc.getFirstValue("id"));
-              + doc.getFirstValue("id"));
-        }
       }
-      
     }
-    if (onlyInB.size() > 0) {
+      
-      for (SolrDocument doc : onlyInB) {
+    for (SolrDocument doc : onlyInB) {
-        if (!deleteFails.contains(doc.getFirstValue("id"))) {
+      if (!deleteFails.contains(doc.getFirstValue("id"))) {
-          legal = false;
+        legal = false;
-        } else {
+      } else {
-          System.err.println("###### Only in " + bName + ": " + onlyInB
+        System.err.println("###### Only in " + bName + ": " + onlyInB
-              + ", but this is expected because we found a delete fail for "
+            + ", but this is expected because we found a delete fail for "
-              + doc.getFirstValue("id"));
+            + doc.getFirstValue("id"));
-        }
       }
     }
     
@@ -1655,8 +1651,12 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes
     if (client == null) {
       final String baseUrl = getBaseUrl((HttpSolrServer) clients.get(clientIndex));
       SolrServer server = createNewSolrServer("", baseUrl);
-      res.setResponse(server.request(request));
+      try {
-      server.shutdown();
+        res.setResponse(server.request(request));
+        server.shutdown();
+      } finally {
+        if (server != null) server.shutdown();
+      }
     } else {
       res.setResponse(client.request(request));
     }