sigram commented on code in PR #2666:
URL: https://github.com/apache/solr/pull/2666#discussion_r1774987714
##########
solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc:
##########
@@ -324,9 +324,20 @@ The default value of this parameter is blank, which causes
no extra "explain inf
This parameter controls Solr's behavior when a query execution limit is
reached (e.g. `timeAllowed` or `cpuAllowed`).
-When this parameter is set to `true` (default) then even though reaching a
limit terminates further query processing Solr will still attempt to return
partial results collected so far. These results may be incomplete in a
non-deterministic way (e.g. only some matching documents, documents without
fields, missing facets or pivots, no spellcheck results, etc).
+When this parameter is set to `true` (default) then Solr will return the
results collected prior to detecting the limit violation.
+If `shards.tolerant=true` is also set, all non-limited responses will still be
collected. If you are seeking a best-effort response use of the
xref:deployment-guide:solrcloud-distributed-requests.adoc#shards-tolerant-parameter[shards.tolerant
Parameter] is recommended.
+
+If incomplete results are returned the `partialResults` response header will
be set to `true`
+
+WARNING: These results may be incomplete in a non-deterministic way (e.g. only
some matching documents, documents without fields, missing facets or pivots, no
spellcheck results, etc).
+
+
+When this parameter is set to `false` then upon reaching a limit, Solr will
stop collecting results and will not return any partial results already
collected.
+In this case, the partialResults response header will be set to `omitted` and
no result documents are returned.
+
+NOTE: The response will be 200 OK because the system has correctly provided
the requested behavior.
+This is not an error condition.
Review Comment:
I like this wording now :) it's clear to me what happens in each case.
##########
solr/core/src/java/org/apache/solr/handler/component/ParallelHttpShardHandler.java:
##########
@@ -45,53 +48,84 @@ public class ParallelHttpShardHandler extends
HttpShardHandler {
private final ExecutorService commExecutor;
+ AtomicInteger attemptStart = new AtomicInteger(0);
+ AtomicInteger attemptCount = new AtomicInteger(0);
+
public ParallelHttpShardHandler(ParallelHttpShardHandlerFactory
httpShardHandlerFactory) {
super(httpShardHandlerFactory);
this.commExecutor = httpShardHandlerFactory.commExecutor;
}
+ @Override
+ protected boolean responsesPending() {
+ // ensure we can't exit while loop in HttpShardHandler.take(boolean) until
we've completed
+ // as many Runnable actions as we created.
+ return super.responsesPending() || attemptStart.get() > attemptCount.get();
+ }
+
@Override
public void submit(ShardRequest sreq, String shard, ModifiableSolrParams
params) {
+ attemptStart.incrementAndGet();
// do this outside of the callable for thread safety reasons
final List<String> urls = getURLs(shard);
final var lbReq = prepareLBRequest(sreq, shard, params, urls);
final var srsp = prepareShardResponse(sreq, shard);
final var ssr = new SimpleSolrResponse();
srsp.setSolrResponse(ssr);
- pending.incrementAndGet();
if (urls.isEmpty()) {
recordNoUrlShardResponse(srsp, shard);
return;
Review Comment:
Not sure if I fully understand the flow here ... wouldn't this throw off the
balance of attemptStart / attemptCount (that we check in `responsesPending()`)
because attemptStart has already been incremented here but the corresponding
attemptCount will never be incremented?
--
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
