From 5ba6c0c7a2ed71fef321abee6b0fee13d93ea183 Mon Sep 17 00:00:00 2001 From: Mikhail Khludnev Date: Wed, 12 Jun 2019 17:53:16 +0200 Subject: [PATCH] SOLR-13509: add omitHeader=false for shards requests to avoid NPE on partialResuls check --- solr/CHANGES.txt | 2 ++ .../handler/component/QueryComponent.java | 26 ++++++++----------- .../solr/handler/component/SearchHandler.java | 7 ++--- .../CloudExitableDirectoryReaderTest.java | 23 +++++++++++----- .../DistributedDebugComponentTest.java | 11 +++----- .../search/facet/RangeFacetCloudTest.java | 9 +++++++ .../src/common-query-parameters.adoc | 22 +++++++++++++++- 7 files changed, 68 insertions(+), 32 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index dabb2d0cf2c..6b677731b9b 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -135,6 +135,8 @@ Bug Fixes * SOLR-12013: collections API CUSTERSTATUS command fails when configset missing (Erick Erickson) +* SOLR-13509: NPE on omitHeader=true is fixed by sending omitHeader=false to shard searches (Munendra S N, Mikhail Khludnev) + Other Changes ---------------------- 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 b0b9fb86b2b..868b5c98892 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 @@ -849,7 +849,7 @@ public class QueryComponent extends SearchComponent } else { responseHeader = (NamedList)srsp.getSolrResponse().getResponse().get("responseHeader"); - final Object rhste = (responseHeader == null ? null : responseHeader.get(SolrQueryResponse.RESPONSE_HEADER_SEGMENT_TERMINATED_EARLY_KEY)); + final Object rhste = responseHeader.get(SolrQueryResponse.RESPONSE_HEADER_SEGMENT_TERMINATED_EARLY_KEY); if (rhste != null) { nl.add(SolrQueryResponse.RESPONSE_HEADER_SEGMENT_TERMINATED_EARLY_KEY, rhste); } @@ -879,20 +879,16 @@ public class QueryComponent extends SearchComponent } final boolean thisResponseIsPartial; - if (responseHeader != null) { - thisResponseIsPartial = Boolean.TRUE.equals(responseHeader.getBooleanArg(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)); - thereArePartialResults |= thisResponseIsPartial; - - if (!Boolean.TRUE.equals(segmentTerminatedEarly)) { - final Object ste = responseHeader.get(SolrQueryResponse.RESPONSE_HEADER_SEGMENT_TERMINATED_EARLY_KEY); - if (Boolean.TRUE.equals(ste)) { - segmentTerminatedEarly = Boolean.TRUE; - } else if (Boolean.FALSE.equals(ste)) { - segmentTerminatedEarly = Boolean.FALSE; - } + thisResponseIsPartial = Boolean.TRUE.equals(responseHeader.getBooleanArg(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)); + thereArePartialResults |= thisResponseIsPartial; + + if (!Boolean.TRUE.equals(segmentTerminatedEarly)) { + final Object ste = responseHeader.get(SolrQueryResponse.RESPONSE_HEADER_SEGMENT_TERMINATED_EARLY_KEY); + if (Boolean.TRUE.equals(ste)) { + segmentTerminatedEarly = Boolean.TRUE; + } else if (Boolean.FALSE.equals(ste)) { + segmentTerminatedEarly = Boolean.FALSE; } - } else { - thisResponseIsPartial = false; } // calculate global maxScore and numDocsFound @@ -1185,7 +1181,7 @@ public class QueryComponent extends SearchComponent } { NamedList responseHeader = (NamedList)srsp.getSolrResponse().getResponse().get("responseHeader"); - if (responseHeader!=null && Boolean.TRUE.equals(responseHeader.getBooleanArg(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY))) { + if (Boolean.TRUE.equals(responseHeader.getBooleanArg(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY))) { rb.rsp.getResponseHeader().asShallowMap() .put(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE); } diff --git a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java index 89cfa2ee53b..96c2200db88 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java @@ -16,9 +16,6 @@ */ package org.apache.solr.handler.component; -import static org.apache.solr.common.params.CommonParams.DISTRIB; -import static org.apache.solr.common.params.CommonParams.PATH; - import java.io.PrintWriter; import java.io.StringWriter; import java.lang.invoke.MethodHandles; @@ -56,6 +53,9 @@ import org.apache.solr.util.plugin.SolrCoreAware; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.apache.solr.common.params.CommonParams.DISTRIB; +import static org.apache.solr.common.params.CommonParams.PATH; + /** * @@ -370,6 +370,7 @@ public class SearchHandler extends RequestHandlerBase implements SolrCoreAware , params.set(ShardParams.IS_SHARD, true); // a sub (shard) request params.set(ShardParams.SHARDS_PURPOSE, sreq.purpose); params.set(ShardParams.SHARD_URL, shard); // so the shard knows what was asked + params.set(CommonParams.OMIT_HEADER, false); if (rb.requestInfo != null) { // we could try and detect when this is needed, but it could be tricky params.set("NOW", Long.toString(rb.requestInfo.getNOW().getTime())); diff --git a/solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java b/solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java index 3a45500d11c..e75d7007dd9 100644 --- a/solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java @@ -21,13 +21,15 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.concurrent.TimeUnit; +import com.carrotsearch.randomizedtesting.annotations.Repeat; +import com.codahale.metrics.Metered; +import com.codahale.metrics.MetricRegistry; import org.apache.lucene.util.TestUtil; import org.apache.solr.client.solrj.embedded.JettySolrRunner; import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.UpdateRequest; import org.apache.solr.client.solrj.response.QueryResponse; import org.apache.solr.cloud.MiniSolrCloudCluster.JettySolrRunnerWithMetrics; -import static org.apache.solr.cloud.TrollingIndexReaderFactory.*; import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.SolrParams; @@ -40,9 +42,11 @@ import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.carrotsearch.randomizedtesting.annotations.Repeat; -import com.codahale.metrics.Metered; -import com.codahale.metrics.MetricRegistry; +import static org.apache.solr.cloud.TrollingIndexReaderFactory.CheckMethodName; +import static org.apache.solr.cloud.TrollingIndexReaderFactory.Trap; +import static org.apache.solr.cloud.TrollingIndexReaderFactory.catchClass; +import static org.apache.solr.cloud.TrollingIndexReaderFactory.catchCount; +import static org.apache.solr.cloud.TrollingIndexReaderFactory.catchTrace; /** * Distributed test for {@link org.apache.lucene.index.ExitableDirectoryReader} @@ -193,7 +197,7 @@ public class CloudExitableDirectoryReaderTest extends SolrCloudTestCase { params( "sort","query($q,1) asc"), params("rows","0", "facet","true", "facet.method", "enum", "facet.field", "name"), params("rows","0", "json.facet","{ ids: { type: range, field : num, start : 1, end : 99, gap : 9 }}") - }; //add more cases here + }; // add more cases here params.add(cases[random().nextInt(cases.length)]); for (; ; creep*=1.5) { @@ -218,13 +222,20 @@ public class CloudExitableDirectoryReaderTest extends SolrCloudTestCase { int numBites = atLeast(100); for(int bite=0; bite track = (NamedList) response.getDebugMap().get("track"); assertNotNull(track); @@ -138,13 +142,6 @@ public class DistributedDebugComponentTest extends SolrJettyTestBase { assertElementsPresent((NamedList)((NamedList)track.get("GET_FIELDS")).get(shard2), "QTime", "ElapsedTime", "RequestPurpose", "NumFound", "Response"); - query.add("omitHeader", "true"); - response = collection1.query(query); - assertNull("QTime is not included in the response when omitHeader is set to true", - ((NamedList)response.getDebugMap().get("track")).findRecursive("EXECUTE_QUERY", shard1, "QTime")); - assertNull("QTime is not included in the response when omitHeader is set to true", - ((NamedList)response.getDebugMap().get("track")).findRecursive("GET_FIELDS", shard2, "QTime")); - query.setQuery("id:1"); response = collection1.query(query); track = (NamedList) response.getDebugMap().get("track"); diff --git a/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java b/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java index 8e53b4e68a3..b11ff3ec9bb 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java +++ b/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java @@ -219,6 +219,15 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { } } } + + public void testStatsWithOmitHeader() throws Exception { + // SOLR-13509: no NPE should be thrown when only stats are specified with omitHeader=true + SolrQuery solrQuery = new SolrQuery("q", "*:*", "omitHeader", "true", + "json.facet", "{unique_foo:\"unique(" + STR_FIELD+ ")\"}"); + final QueryResponse rsp = cluster.getSolrClient().query(solrQuery); + // response shouldn't contain header as omitHeader is set to true + assertNull(rsp.getResponseHeader()); + } public void testInclude_Upper() throws Exception { for (boolean doSubFacet : Arrays.asList(false, true)) { diff --git a/solr/solr-ref-guide/src/common-query-parameters.adoc b/solr/solr-ref-guide/src/common-query-parameters.adoc index 499401525c0..aeb77df3c01 100644 --- a/solr/solr-ref-guide/src/common-query-parameters.adoc +++ b/solr/solr-ref-guide/src/common-query-parameters.adoc @@ -203,7 +203,27 @@ The default value of this parameter is blank, which causes no extra "explain inf == timeAllowed Parameter -This parameter specifies the amount of time, in milliseconds, allowed for a search to complete. If this time expires before the search is complete, any partial results will be returned, but values such as `numFound`, <> counts, and result <> may not be accurate for the entire result set. +This parameter specifies the amount of time, in milliseconds, allowed for a search to complete. If this time expires before the search is complete, any partial results will be returned, but values such as `numFound`, <> counts, and result <> may not be accurate for the entire result set. In case of expiration, if `omitHeader` isn't set to `true` the response header contains a special flag called `partialResults`. + +[source,json] +---- +{ + "responseHeader": { + "status": 0, + "zkConnected": true, + "partialResults": true, + "QTime": 20, + "params": { + "q": "*:*" + } + }, + "response": { + "numFound": 77, + "start": 0, + "docs": [ "..." ] + } +} +---- This value is only checked at the time of: