From 271576ed0f81421b0717f6da23bf3371c6c1bd87 Mon Sep 17 00:00:00 2001 From: Shalin Shekhar Mangar Date: Fri, 4 Jul 2014 17:54:53 +0000 Subject: [PATCH] SOLR-6223: SearchComponents may throw NPE when using shards.tolerant and there is a failure in the 'GET_FIELDS/GET_HIGHLIGHTS/GET_DEBUG' phase git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1607897 13f79535-47bb-0310-9956-ffa450edef68 --- solr/CHANGES.txt | 3 + .../handler/component/DebugComponent.java | 2 +- .../handler/component/HighlightComponent.java | 7 +- .../component/MoreLikeThisComponent.java | 4 + .../handler/component/QueryComponent.java | 27 +- .../component/TermVectorComponent.java | 4 +- .../org/apache/solr/util/SolrPluginUtils.java | 28 +- .../conf/solrconfig-tolerant-search.xml | 57 +++++ .../org/apache/solr/TestTolerantSearch.java | 241 ++++++++++++++++++ 9 files changed, 355 insertions(+), 18 deletions(-) create mode 100644 solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-search.xml create mode 100644 solr/core/src/test/org/apache/solr/TestTolerantSearch.java diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 0614b5ebcbb..e654c02a84e 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -155,6 +155,9 @@ Bug Fixes * SOLR-6159: A ZooKeeper session expiry during setup can keep LeaderElector from joining elections. (Steven Bower, shalin) +* SOLR-6223: SearchComponents may throw NPE when using shards.tolerant and there is a failure + in the 'GET_FIELDS/GET_HIGHLIGHTS/GET_DEBUG' phase. (Tomás Fernández Löbbe via shalin) + Other Changes --------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/component/DebugComponent.java b/solr/core/src/java/org/apache/solr/handler/component/DebugComponent.java index 21d6ff59e6a..9a206cf8726 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/DebugComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/DebugComponent.java @@ -230,7 +230,7 @@ public class DebugComponent extends SearchComponent } if (rb.isDebugResults()) { - explain = SolrPluginUtils.removeNulls(new SimpleOrderedMap<>(arr)); + explain = SolrPluginUtils.removeNulls(arr, new SimpleOrderedMap<>()); } if (!hasGetDebugResponses) { diff --git a/solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java b/solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java index 2bf24a261a1..87746c779d5 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java @@ -182,6 +182,11 @@ public class HighlightComponent extends SearchComponent implements PluginInfoIni for (ShardRequest sreq : rb.finished) { if ((sreq.purpose & ShardRequest.PURPOSE_GET_HIGHLIGHTS) == 0) continue; for (ShardResponse srsp : sreq.responses) { + if (srsp.getException() != null) { + // can't expect the highlight content if there was an exception for this request + // this should only happen when using shards.tolerant=true + continue; + } NamedList hl = (NamedList)srsp.getSolrResponse().getResponse().get("highlighting"); for (int i=0; i())); } } diff --git a/solr/core/src/java/org/apache/solr/handler/component/MoreLikeThisComponent.java b/solr/core/src/java/org/apache/solr/handler/component/MoreLikeThisComponent.java index c53b5424a28..7df06b8cf54 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/MoreLikeThisComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/MoreLikeThisComponent.java @@ -133,6 +133,10 @@ public class MoreLikeThisComponent extends SearchComponent { && rb.req.getParams().getBool(COMPONENT_NAME, false)) { log.debug("ShardRequest.response.size: " + sreq.responses.size()); for (ShardResponse r : sreq.responses) { + if (r.getException() != null) { + // This should only happen in case of using shards.tolerant=true. Omit this ShardResponse + continue; + } NamedList moreLikeThisReponse = (NamedList) r.getSolrResponse() .getResponse().get("moreLikeThis"); log.debug("ShardRequest.response.shard: " + r.getShard()); diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java index 59ca82fbd47..f6cf8a04d0b 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java @@ -55,7 +55,6 @@ import org.apache.solr.common.SolrDocument; import org.apache.solr.common.SolrDocumentList; import org.apache.solr.common.SolrException; import org.apache.solr.common.params.*; -import org.apache.solr.common.params.CursorMarkParams; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.SimpleOrderedMap; import org.apache.solr.common.util.StrUtils; @@ -1064,7 +1063,9 @@ public class QueryComponent extends SearchComponent populateNextCursorMarkFromMergedShards(rb); if (partialResults) { - rb.rsp.getResponseHeader().add( "partialResults", Boolean.TRUE ); + if(rb.rsp.getResponseHeader().get("partialResults") == null) { + rb.rsp.getResponseHeader().add("partialResults", Boolean.TRUE); + } } } @@ -1227,6 +1228,28 @@ public class QueryComponent extends SearchComponent boolean removeKeyField = !rb.rsp.getReturnFields().wantsField(keyFieldName); for (ShardResponse srsp : sreq.responses) { + if (srsp.getException() != null) { + // Don't try to get the documents if there was an exception in the shard + if(rb.req.getParams().getBool(ShardParams.SHARDS_INFO, false)) { + @SuppressWarnings("unchecked") + NamedList shardInfo = (NamedList) rb.rsp.getValues().get(ShardParams.SHARDS_INFO); + @SuppressWarnings("unchecked") + SimpleOrderedMap nl = (SimpleOrderedMap) shardInfo.get(srsp.getShard()); + if (nl.get("error") == null) { + // Add the error to the shards info section if it wasn't added before + Throwable t = srsp.getException(); + if(t instanceof SolrServerException) { + t = ((SolrServerException)t).getCause(); + } + nl.add("error", t.toString() ); + StringWriter trace = new StringWriter(); + t.printStackTrace(new PrintWriter(trace)); + nl.add("trace", trace.toString() ); + } + } + + continue; + } SolrDocumentList docs = (SolrDocumentList) srsp.getSolrResponse().getResponse().get("response"); for (SolrDocument doc : docs) { diff --git a/solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java b/solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java index 2b579bc2ebc..ced30770180 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/TermVectorComponent.java @@ -425,7 +425,7 @@ public class TermVectorComponent extends SearchComponent implements SolrCoreAwar public void finishStage(ResponseBuilder rb) { if (rb.stage == ResponseBuilder.STAGE_GET_FIELDS) { - NamedList termVectors = new NamedList<>(); + NamedList termVectors = new NamedList<>(); Map.Entry[] arr = new NamedList.NamedListEntry[rb.resultIds.size()]; for (ShardRequest sreq : rb.finished) { @@ -450,7 +450,7 @@ public class TermVectorComponent extends SearchComponent implements SolrCoreAwar } } // remove nulls in case not all docs were able to be retrieved - termVectors.addAll(SolrPluginUtils.removeNulls(new NamedList<>(arr))); + termVectors.addAll(SolrPluginUtils.removeNulls(arr, new NamedList())); rb.rsp.add(TERM_VECTORS, termVectors); } } diff --git a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java index 75be77bada3..dcad4022711 100644 --- a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java +++ b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java @@ -702,21 +702,25 @@ public class SolrPluginUtils { } return s.toString().replace("\"",""); } - - public static NamedList removeNulls(NamedList nl) { - for (int i=0; i NamedList removeNulls(Map.Entry[] entries, NamedList dest) { + for (int i=0; i entry = entries[i]; + if (entry != null) { + String key = entry.getKey(); + if (key != null) { + dest.add(key, entry.getValue()); } - return newList; } } - return nl; + return dest; } /** diff --git a/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-search.xml b/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-search.xml new file mode 100644 index 00000000000..cb1ab3df944 --- /dev/null +++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig-tolerant-search.xml @@ -0,0 +1,57 @@ + + + + + + + + + ${solr.data.dir:} + + + + ${tests.luceneMatchVersion:LUCENE_CURRENT} + + + + + + ${solr.commitwithin.softcommit:true} + + + + + + explicit + true + text + + + + + + + + + + + diff --git a/solr/core/src/test/org/apache/solr/TestTolerantSearch.java b/solr/core/src/test/org/apache/solr/TestTolerantSearch.java new file mode 100644 index 00000000000..81f65ad55da --- /dev/null +++ b/solr/core/src/test/org/apache/solr/TestTolerantSearch.java @@ -0,0 +1,241 @@ +package org.apache.solr; + +import java.io.File; +import java.io.IOException; +import java.io.OutputStream; + +import org.apache.commons.io.FileUtils; +import org.apache.solr.client.solrj.SolrQuery; +import org.apache.solr.client.solrj.SolrServer; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.client.solrj.impl.HttpSolrServer; +import org.apache.solr.client.solrj.request.CoreAdminRequest; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.ShardParams; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.BinaryResponseWriter; +import org.apache.solr.response.SolrQueryResponse; +import org.junit.AfterClass; +import org.junit.BeforeClass; + +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class TestTolerantSearch extends SolrJettyTestBase { + + private static SolrServer collection1; + private static SolrServer collection2; + private static String shard1; + private static String shard2; + private static File solrHome; + + private static File createSolrHome() throws Exception { + File workDir = createTempDir(); + setupJettyTestHome(workDir, "collection1"); + FileUtils.copyFile(new File(SolrTestCaseJ4.TEST_HOME() + "/collection1/conf/solrconfig-tolerant-search.xml"), new File(workDir, "/collection1/conf/solrconfig.xml")); + FileUtils.copyDirectory(new File(workDir, "collection1"), new File(workDir, "collection2")); + return workDir; + } + + + @BeforeClass + public static void createThings() throws Exception { + solrHome = createSolrHome(); + createJetty(solrHome.getAbsolutePath(), null, null); + String url = jetty.getBaseUrl().toString(); + collection1 = new HttpSolrServer(url); + collection2 = new HttpSolrServer(url + "/collection2"); + + String urlCollection1 = jetty.getBaseUrl().toString() + "/" + "collection1"; + String urlCollection2 = jetty.getBaseUrl().toString() + "/" + "collection2"; + shard1 = urlCollection1.replaceAll("https?://", ""); + shard2 = urlCollection2.replaceAll("https?://", ""); + + //create second core + CoreAdminRequest.Create req = new CoreAdminRequest.Create(); + req.setCoreName("collection2"); + collection1.request(req); + + SolrInputDocument doc = new SolrInputDocument(); + doc.setField("id", "1"); + doc.setField("subject", "batman"); + doc.setField("title", "foo bar"); + collection1.add(doc); + collection1.commit(); + + doc.setField("id", "2"); + doc.setField("subject", "superman"); + collection2.add(doc); + collection2.commit(); + + doc = new SolrInputDocument(); + doc.setField("id", "3"); + doc.setField("subject", "aquaman"); + doc.setField("title", "foo bar"); + collection1.add(doc); + collection1.commit(); + + } + + @AfterClass + public static void destroyThings() throws Exception { + collection1.shutdown(); + collection2.shutdown(); + collection1 = null; + collection2 = null; + jetty.stop(); + jetty=null; + resetExceptionIgnores(); + } + + @SuppressWarnings("unchecked") + public void testGetFieldsPhaseError() throws SolrServerException { + BadResponseWriter.failOnGetFields = true; + BadResponseWriter.failOnGetTopIds = false; + SolrQuery query = new SolrQuery(); + query.setQuery("subject:batman OR subject:superman"); + query.addField("id"); + query.addField("subject"); + query.set("distrib", "true"); + query.set("shards", shard1 + "," + shard2); + query.set(ShardParams.SHARDS_INFO, "true"); + query.set("debug", "true"); + query.set("stats", "true"); + query.set("stats.field", "id"); + query.set("mlt", "true"); + query.set("mlt.fl", "title"); + query.set("mlt.count", "1"); + query.set("mlt.mintf", "0"); + query.set("mlt.mindf", "0"); + query.setHighlight(true); + query.addFacetField("id"); + query.setFacet(true); + + ignoreException("Dummy exception in BadResponseWriter"); + try { + collection1.query(query); + fail("Should get an exception"); + } catch (Exception e) { + //expected + } + query.set(ShardParams.SHARDS_TOLERANT, "true"); + QueryResponse response = collection1.query(query); + assertTrue(response.getResponseHeader().getBooleanArg("partialResults")); + NamedList shardsInfo = ((NamedList)response.getResponse().get("shards.info")); + boolean foundError = false; + for (int i = 0; i < shardsInfo.size(); i++) { + if (shardsInfo.getName(i).contains("collection2")) { + assertNotNull(((NamedList)shardsInfo.getVal(i)).get("error")); + foundError = true; + break; + } + } + assertTrue(foundError); + assertEquals(1, response.getResults().get(0).getFieldValue("id")); + assertEquals("batman", response.getResults().get(0).getFirstValue("subject")); + unIgnoreException("Dummy exception in BadResponseWriter"); + } + + @SuppressWarnings("unchecked") + public void testGetTopIdsPhaseError() throws SolrServerException { + BadResponseWriter.failOnGetTopIds = true; + BadResponseWriter.failOnGetFields = false; + SolrQuery query = new SolrQuery(); + query.setQuery("subject:batman OR subject:superman"); + query.addField("id"); + query.addField("subject"); + query.set("distrib", "true"); + query.set("shards", shard1 + "," + shard2); + query.set(ShardParams.SHARDS_INFO, "true"); + query.set("debug", "true"); + query.set("stats", "true"); + query.set("stats.field", "id"); + query.set("mlt", "true"); + query.set("mlt.fl", "title"); + query.set("mlt.count", "1"); + query.set("mlt.mintf", "0"); + query.set("mlt.mindf", "0"); + query.setHighlight(true); + query.addFacetField("id"); + query.setFacet(true); + + ignoreException("Dummy exception in BadResponseWriter"); + try { + collection1.query(query); + fail("Should get an exception"); + } catch (Exception e) { + //expected + } + query.set(ShardParams.SHARDS_TOLERANT, "true"); + QueryResponse response = collection1.query(query); + assertTrue(response.getResponseHeader().getBooleanArg("partialResults")); + NamedList shardsInfo = ((NamedList)response.getResponse().get("shards.info")); + boolean foundError = false; + for (int i = 0; i < shardsInfo.size(); i++) { + if (shardsInfo.getName(i).contains("collection2")) { + assertNotNull(((NamedList)shardsInfo.getVal(i)).get("error")); + foundError = true; + break; + } + } + assertTrue(foundError); + + assertEquals(1, response.getResults().get(0).getFieldValue("id")); + assertEquals("batman", response.getResults().get(0).getFirstValue("subject")); + unIgnoreException("Dummy exception in BadResponseWriter"); + } + + public static class BadResponseWriter extends BinaryResponseWriter { + + private static boolean failOnGetFields = false; + private static boolean failOnGetTopIds = false; + + public BadResponseWriter() { + super(); + } + + @Override + public void write(OutputStream out, SolrQueryRequest req, + SolrQueryResponse response) throws IOException { + + // I want to fail on the shard request, not the original user request, and only on the + // GET_FIELDS phase + if (failOnGetFields && + "collection2".equals(req.getCore().getName()) + && "subject:batman OR subject:superman".equals(req.getParams().get("q", "")) + && req.getParams().get("ids") != null) { + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, + "Dummy exception in BadResponseWriter"); + } else if (failOnGetTopIds + && "collection2".equals(req.getCore().getName()) + && "subject:batman OR subject:superman".equals(req.getParams().get("q", "")) + && req.getParams().get("ids") == null + && req.getParams().getBool("isShard", false) == true) { + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, + "Dummy exception in BadResponseWriter"); + } + super.write(out, req, response); + } + + + } + + +}