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