sigram commented on code in PR #2379:
URL: https://github.com/apache/solr/pull/2379#discussion_r1547460728
##########
solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java:
##########
@@ -118,12 +122,27 @@ public interface ShardParams {
* <p>By default, returns <code>false</code> when {@value #SHARDS_TOLERANT}
is not set in <code>
* params</code>.
*/
- static boolean getShardsTolerantAsBool(SolrParams params) {
- String shardsTolerantValue = params.get(SHARDS_TOLERANT);
+ static boolean getShardsTolerantAsBool(RequestParamsSupplier req) {
+ String shardsTolerantValue = req.getParams().get(SHARDS_TOLERANT);
if (null == shardsTolerantValue ||
shardsTolerantValue.equals(REQUIRE_ZK_CONNECTED)) {
return false;
} else {
- return StrUtils.parseBool(shardsTolerantValue); // throw an exception if
non-boolean
+ boolean tolerant = StrUtils.parseBool(shardsTolerantValue);
+ if (tolerant && req.shouldDiscardPartials()) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "Use of "
+ + SHARDS_TOLERANT
+ + " requires that "
+ + ALLOW_PARTIAL_RESULTS
+ + " is true. If "
+ + ALLOW_PARTIAL_RESULTS
+ + " is defaulted to true for this handler or system wide,
explicitly"
+ + "passing "
+ + ALLOW_PARTIAL_RESULTS
+ + "=true will allow shards.tolerant to work");
Review Comment:
I don't understand this message... maybe it's missing a negation somewhere?
This would make more sense to me: "If ALLOW_PARTIAL_RESULTS defaults to
FALSE... then passing ALLOW_PARTIAL_RESULTS=true ..."
##########
solr/solrj/src/java/org/apache/solr/common/params/CommonParams.java:
##########
@@ -228,6 +228,7 @@ public interface CommonParams {
METRICS_PATH);
String APISPEC_LOCATION = "apispec/";
String INTROSPECT = "/_introspect";
+ String ALLOW_PARTIAL_RESULTS = "allowPartialResults";
Review Comment:
This should be replaced with (or used instead of)
`CommonParams.PARTIAL_RESULTS`, and the associated docs in the ref guide needs
to be changed accordingly.
These two parameters carry the exactly same meaning and serve the same
purpose - i.e. that returning partial results is either acceptable or not. For
QueryLimits if it's not acceptable (`partialResults=false`, whichever way this
value was set) and the query limits are exceeded then partial results will be
discarded by throwing an exception inside components (which is implemented in
`QueryLimits.maybeExitWithPartialResults`). And here exactly the same behavior
is expected, consistent with `CommonParams.PARTIAL_RESULTS`.
##########
solr/core/src/java/org/apache/solr/request/json/RequestUtil.java:
##########
@@ -216,24 +222,26 @@ public static void processParams(
boolean isQuery = false;
boolean arr = false;
if ("query".equals(key)) {
- out = "q";
+ out = Q;
isQuery = true;
// if the value is not a String, then it'll get converted to a
localParams query String.
// Only the "lucene" query parser can parse it. Ignore anything
else that may exist.
if (!(entry.getValue() instanceof String)) {
newMap.put(QueryParsing.DEFTYPE, new String[] {"lucene"});
}
} else if ("filter".equals(key)) {
- out = "fq";
+ out = FQ;
arr = true;
isQuery = true;
} else if ("fields".equals(key)) {
- out = "fl";
+ out = FL;
arr = true;
} else if ("offset".equals(key)) {
- out = "start";
+ out = START;
+ } else if ("allowPartialResults".equals(key)) {
Review Comment:
Why is this needed if they are the same value?
##########
solr/core/src/java/org/apache/solr/search/Grouping.java:
##########
@@ -461,7 +463,7 @@ private void searchWithTimeLimiter(final Query filterQuery,
Collector collector)
log.info("Query: {}; ", query);
// to make WARN logged exception content more visible
log.warn("Query: {}; ", query.getClass().getName(), x);
- qr.setPartialResults(true);
+ qr.setPartialResults(shouldKeepPartials());
Review Comment:
Hmm, I'm not sure if this is correct - when these exceptions are thrown we
know for sure that results are incomplete. Why would we set
`partialResults=false` here? ? Unless there's an implicit behavior here that if
we discard the results then we (always?) return `partialResults=false`.
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -140,58 +150,64 @@ public void submit(
srsp.setShard(shard);
SimpleSolrResponse ssr = new SimpleSolrResponse();
srsp.setSolrResponse(ssr);
+ synchronized (RESPONSE_CANCELABLE_LOCK) {
+ pending.incrementAndGet();
+ // if there are no shards available for a slice, urls.size()==0
+ if (urls.isEmpty()) {
+ // TODO: what's the right error code here? We should use the same
thing when
+ // all of the servers for a shard are down.
+ SolrException exception =
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE, "no servers
hosting shard: " + shard);
+ srsp.setException(exception);
+ srsp.setResponseCode(exception.code());
+ responses.add(srsp);
+ return;
+ }
- pending.incrementAndGet();
- // if there are no shards available for a slice, urls.size()==0
- if (urls.isEmpty()) {
- // TODO: what's the right error code here? We should use the same thing
when
- // all of the servers for a shard are down.
- SolrException exception =
- new SolrException(
- SolrException.ErrorCode.SERVICE_UNAVAILABLE, "no servers hosting
shard: " + shard);
- srsp.setException(exception);
- srsp.setResponseCode(exception.code());
- responses.add(srsp);
- return;
- }
+ // all variables that set inside this listener must be at least volatile
+ responseCancellableMap.put(
Review Comment:
Maybe if we used a `ConcurrentHashMap` we could avoid locking here
altogether?
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -497,7 +495,12 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
debug.add("explain", new NamedList<>());
rb.rsp.add("debug", debug);
}
- rb.rsp.setPartialResults();
+ if (req.shouldDiscardPartials()) {
+ // TODO: this isn't actually showing up... possibly need to do tis
at a different point.
+ rb.rsp.getResponseHeader().add("partialResultsOmitted", "true");
Review Comment:
Maybe `partialResultsDiscarded` ?
##########
solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java:
##########
@@ -224,7 +226,7 @@ public NamedList<NamedList<Object>> processResult(
if (docSet != null) {
queryResult.setDocSet(docSet);
}
- queryResult.setPartialResults(partialResults);
+ queryResult.setPartialResults(partialResults && shouldKeepPartials());
Review Comment:
See above.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/RequestParamsSupplier.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.
+ */
+package org.apache.solr.client.solrj.request;
+
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.SolrParams;
+
+/** Interface to coordinate param parsing logic when solrj and solrcore both
use a parameter */
+// TODO: I've just realized that the reason I introduced this class
(getShardsTolerantAsBool
+// in ShardParams) is never actually used in solrj code, and only used in
server side code
+// despite living in solrj... so maybe I'll just move that method instead...
+public interface RequestParamsSupplier {
+
+ // TODO: Think about use cases, maybe get rid of sysprop?
+ // For per-index/collection usage setting the allowPartialResults request
param in defaults
+ // for the handler is preferred, but for large installations with thousands
of tenants, possibly
+ // with
+ // variations in their solrconfg.xml that prevent simple search/replace
this would be
+ // impractical. Thus a means of setting a global default seems appropriate.
System property is
+ // a quick and dirty way but In "user managed" mode it could possibly be a
solr.xml (user's
+ // responsibility to manage, similar to a system property), and in cloud
mode this maybe should
+ // hinge on a cluster property since there is no conceivable reason to
have individual nodes
+ // with differing defaults. For the cloud case we probably would want to
ignore the setting in
+ // solr.xml to guard against inconsistent shard behavior due to
misconfiguration. A further
+ // consideration is how things like a docker compose or kubernetes system
ought to be able
+ // to initialize cluster properties like this for consistent
non-interactive deployment...
+ String SOLR_DISLIKE_PARTIAL_RESULTS = "solr.dislike.partial.results";
Review Comment:
Hmm, a common way to solve this without using a "magic" order of preference
between sysprops and query params is to use property substitution in config
files, be it solr.xml or solrconfig.xml. Whatever is the process to start Solr,
either a CLI or Kubernetes deployment, or Solr Operator, you can then pass an
env var to set it consistently to the same value cluster-wide in e.g.
solrconfig.xml invariants section of the search handler. So I'm not convinced
why we need a dedicated sysprop for ensuring consistency.
Also, IMHO the property name is confusing because it suggests some
indeterminate soft preference, while it's a straight hard boolean default - why
not use the same name as the param, plus the prefix? I.e.
`solr.allow.partial.results` - and then document the behavior when both the
sysprop and the query param (either explicit or implicit via invariants /
defaults) are specified.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/util/AsyncListener.java:
##########
@@ -19,10 +19,23 @@
/** Listener for async requests */
public interface AsyncListener<T> {
- /** Callback method invoked before processing the request */
+ /**
+ * Setup method invoked in the original thread before processing the request
on a different
Review Comment:
💯 very important clarification that explains why we're later getting the
right `SolrRequestInfo`.
##########
solr/core/src/test/org/apache/solr/search/TestCpuAllowedLimit.java:
##########
@@ -215,4 +225,116 @@ public void testDistribLimit2() throws Exception {
// https://issues.apache.org/jira/browse/SOLR-17203
testDistribLimit();
}
+
+ public void testDistribLimitNoPartialParam() throws Exception {
+ Assume.assumeTrue("Thread CPU time monitoring is not available",
ThreadCpuTimer.isSupported());
+
+ SolrClient solrClient = cluster.getSolrClient();
+
+ // no limits set - should eventually complete
+ log.info("--- No limits, full results ---");
+ long sleepMs = 1000;
+ QueryResponse rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id asc",
+ ExpensiveSearchComponent.SLEEP_MS_PARAM,
+ String.valueOf(sleepMs),
+ "stages",
+ "prepare,process"));
+ // System.err.println("rsp=" + rsp.jsonStr());
+ assertEquals(rsp.getHeader().get("status"), 0);
+ Number qtime = (Number) rsp.getHeader().get("QTime");
+ assertTrue("QTime expected " + qtime + " >> " + sleepMs, qtime.longValue()
> sleepMs);
+ assertNull("should not have partial results",
rsp.getHeader().get("partialResults"));
+
+ // timeAllowed set, should return partial results
Review Comment:
This comment is incorrect.
##########
solr/core/src/test/org/apache/solr/search/TestCpuAllowedLimit.java:
##########
@@ -215,4 +225,116 @@ public void testDistribLimit2() throws Exception {
// https://issues.apache.org/jira/browse/SOLR-17203
testDistribLimit();
}
+
+ public void testDistribLimitNoPartialParam() throws Exception {
+ Assume.assumeTrue("Thread CPU time monitoring is not available",
ThreadCpuTimer.isSupported());
+
+ SolrClient solrClient = cluster.getSolrClient();
+
+ // no limits set - should eventually complete
+ log.info("--- No limits, full results ---");
+ long sleepMs = 1000;
+ QueryResponse rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id asc",
+ ExpensiveSearchComponent.SLEEP_MS_PARAM,
+ String.valueOf(sleepMs),
+ "stages",
+ "prepare,process"));
+ // System.err.println("rsp=" + rsp.jsonStr());
+ assertEquals(rsp.getHeader().get("status"), 0);
+ Number qtime = (Number) rsp.getHeader().get("QTime");
+ assertTrue("QTime expected " + qtime + " >> " + sleepMs, qtime.longValue()
> sleepMs);
+ assertNull("should not have partial results",
rsp.getHeader().get("partialResults"));
+
+ // timeAllowed set, should return partial results
+ log.info("--- timeAllowed expires, but should not have partial results
---");
+ rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id asc",
+ ExpensiveSearchComponent.SLEEP_MS_PARAM,
+ String.valueOf(sleepMs),
+ "stages",
+ "prepare,process",
+ "timeAllowed",
+ "500"));
+ assertNull("should not have partial results",
rsp.getHeader().get("partialResults"));
+ assertEquals(0, rsp.getResults().size());
+
+ // cpuAllowed set with large value, should return full results
+ log.info("--- cpuAllowed, full results ---");
+ rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id desc",
+ ExpensiveSearchComponent.CPU_LOAD_COUNT_PARAM,
+ "1",
+ "stages",
+ "prepare,process",
+ "cpuAllowed",
+ "1000"));
+ assertNull("should have full results",
rsp.getHeader().get("partialResults"));
+
+ // cpuAllowed set, should return partial results
+ log.info("--- cpuAllowed 1, partial results ---");
+ rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id desc",
+ ExpensiveSearchComponent.CPU_LOAD_COUNT_PARAM,
+ "10",
+ "stages",
+ "prepare,process",
+ "cpuAllowed",
+ "50"));
+ assertNull("should not have partial results",
rsp.getHeader().get("partialResults"));
+ assertEquals(0, rsp.getResults().size());
+
+ // cpuAllowed set, should return partial results
Review Comment:
Ditto.
##########
solr/core/src/test/org/apache/solr/search/TestCpuAllowedLimit.java:
##########
@@ -215,4 +225,116 @@ public void testDistribLimit2() throws Exception {
// https://issues.apache.org/jira/browse/SOLR-17203
testDistribLimit();
}
+
+ public void testDistribLimitNoPartialParam() throws Exception {
+ Assume.assumeTrue("Thread CPU time monitoring is not available",
ThreadCpuTimer.isSupported());
+
+ SolrClient solrClient = cluster.getSolrClient();
+
+ // no limits set - should eventually complete
+ log.info("--- No limits, full results ---");
+ long sleepMs = 1000;
+ QueryResponse rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id asc",
+ ExpensiveSearchComponent.SLEEP_MS_PARAM,
+ String.valueOf(sleepMs),
+ "stages",
+ "prepare,process"));
+ // System.err.println("rsp=" + rsp.jsonStr());
+ assertEquals(rsp.getHeader().get("status"), 0);
+ Number qtime = (Number) rsp.getHeader().get("QTime");
+ assertTrue("QTime expected " + qtime + " >> " + sleepMs, qtime.longValue()
> sleepMs);
+ assertNull("should not have partial results",
rsp.getHeader().get("partialResults"));
+
+ // timeAllowed set, should return partial results
+ log.info("--- timeAllowed expires, but should not have partial results
---");
+ rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id asc",
+ ExpensiveSearchComponent.SLEEP_MS_PARAM,
+ String.valueOf(sleepMs),
+ "stages",
+ "prepare,process",
+ "timeAllowed",
+ "500"));
+ assertNull("should not have partial results",
rsp.getHeader().get("partialResults"));
+ assertEquals(0, rsp.getResults().size());
+
+ // cpuAllowed set with large value, should return full results
+ log.info("--- cpuAllowed, full results ---");
+ rsp =
+ solrClient.query(
+ COLLECTION,
+ params(
+ "q",
+ "id:*",
+ "allowPartialResults",
+ "false",
+ "sort",
+ "id desc",
+ ExpensiveSearchComponent.CPU_LOAD_COUNT_PARAM,
+ "1",
+ "stages",
+ "prepare,process",
+ "cpuAllowed",
+ "1000"));
+ assertNull("should have full results",
rsp.getHeader().get("partialResults"));
+
+ // cpuAllowed set, should return partial results
Review Comment:
Ditto.
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -276,8 +278,8 @@ private Collector buildAndRunCollectorChain(
} catch (TimeLimitingCollector.TimeExceededException
| ExitableDirectoryReader.ExitingReaderException
| CancellableCollector.QueryCancelledException x) {
- log.warn("Query: [{}]; ", query, x);
- qr.setPartialResults(true);
+ log.warn("Query: [{}]; was timed out or canceled.", query, x);
+ qr.setPartialResults(shouldKeepPartials());
Review Comment:
See above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
