diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index e237bc4286a..45eeb119912 100644 --- a/solr/CHANGES.txt +++ b/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 diff --git a/solr/core/src/test/org/apache/solr/cloud/ChaosMonkeyNothingIsSafeTest.java b/solr/core/src/test/org/apache/solr/cloud/ChaosMonkeyNothingIsSafeTest.java index 9803470e25b..9f2d84ca7a8 100644 --- a/solr/core/src/test/org/apache/solr/cloud/ChaosMonkeyNothingIsSafeTest.java +++ b/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"}; - public static RandVal[] randVals = new RandVal[]{rint, rfloat, rdouble, rlong, rdate}; + protected static final String[] fieldNames = new String[]{"f_i", "f_f", "f_d", "f_l", "f_dt"}; + 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; } diff --git a/solr/core/src/test/org/apache/solr/cloud/ChaosMonkeySafeLeaderTest.java b/solr/core/src/test/org/apache/solr/cloud/ChaosMonkeySafeLeaderTest.java index 19ae3ed13a7..40137955ad3 100644 --- a/solr/core/src/test/org/apache/solr/cloud/ChaosMonkeySafeLeaderTest.java +++ b/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"}; - public static RandVal[] randVals = new RandVal[]{rint, rfloat, rdouble, rlong, rdate}; + protected static final String[] fieldNames = new String[]{"f_i", "f_f", "f_d", "f_l", "f_dt"}; + 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; } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrServer.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrServer.java index a1ebefe9ddd..858795719bf 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrServer.java +++ b/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 { - rsp.rsp = server.request(req.getRequest()); + ex = doRequest(server, req, rsp, isUpdate, false, null); + 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 { - rsp.rsp = wrapper.solrServer.request(req.getRequest()); - zombieServers.remove(wrapper.getKey()); - 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); + ex = doRequest(wrapper.solrServer, req, rsp, isUpdate, true, wrapper.getKey()); + if (ex == null) { + return rsp; // SUCCESS } } @@ -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) { diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java index afbe5cd971d..b11f9088879 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java @@ -1137,27 +1137,23 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes Set onlyInB = new HashSet<>(setB); onlyInB.removeAll(setA); - if (onlyInA.size() > 0) { - for (SolrDocument doc : onlyInA) { - if (!addFails.contains(doc.getFirstValue("id"))) { - legal = false; - } else { - System.err.println("###### Only in " + aName + ": " + onlyInA - + ", but this is expected because we found an add fail for " - + doc.getFirstValue("id")); - } + for (SolrDocument doc : onlyInA) { + if (!addFails.contains(doc.getFirstValue("id"))) { + legal = false; + } else { + System.err.println("###### Only in " + aName + ": " + onlyInA + + ", but this is expected because we found an add fail for " + + doc.getFirstValue("id")); } - } - if (onlyInB.size() > 0) { - for (SolrDocument doc : onlyInB) { - if (!deleteFails.contains(doc.getFirstValue("id"))) { - legal = false; - } else { - System.err.println("###### Only in " + bName + ": " + onlyInB - + ", but this is expected because we found a delete fail for " - + doc.getFirstValue("id")); - } + + for (SolrDocument doc : onlyInB) { + if (!deleteFails.contains(doc.getFirstValue("id"))) { + legal = false; + } else { + System.err.println("###### Only in " + bName + ": " + onlyInB + + ", but this is expected because we found a delete fail for " + + 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)); - server.shutdown(); + try { + res.setResponse(server.request(request)); + server.shutdown(); + } finally { + if (server != null) server.shutdown(); + } } else { res.setResponse(client.request(request)); }