dsmiley commented on code in PR #2666: URL: https://github.com/apache/solr/pull/2666#discussion_r1736962583
########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -42,13 +45,17 @@ import org.apache.solr.common.params.ShardParams; import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.StrUtils; import org.apache.solr.core.CoreDescriptor; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.request.SolrRequestInfo; import org.apache.solr.security.AllowListUrlChecker; @NotThreadSafe public class HttpShardHandler extends ShardHandler { + /** */ Review Comment: either say something or don't :-) ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -80,6 +94,39 @@ public HttpShardHandler(HttpShardHandlerFactory httpShardHandlerFactory) { shardToURLs = new HashMap<>(); } + /** + * Parse the {@value ShardParams#SHARDS_TOLERANT} param from <code>params</code> as a boolean; + * accepts {@value ShardParams#REQUIRE_ZK_CONNECTED} as a valid value indicating <code>false + * </code>. + * + * <p>By default, returns <code>false</code> when {@value ShardParams#SHARDS_TOLERANT} is not set + * in <code> + * params</code>. + */ + public static boolean getShardsTolerantAsBool(SolrQueryRequest req) { + String shardsTolerantValue = req.getParams().get(ShardParams.SHARDS_TOLERANT); + if (null == shardsTolerantValue + || shardsTolerantValue.trim().equals(ShardParams.REQUIRE_ZK_CONNECTED)) { Review Comment: we don't normally trim our params. I think it's very haphazard to do it on-read (like once we do it here and there, then everywhere we wonder, should we do here too? a mess IMO) ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -62,7 +69,14 @@ public class HttpShardHandler extends ShardHandler { private HttpShardHandlerFactory httpShardHandlerFactory; private Map<ShardResponse, CompletableFuture<LBSolrClient.Rsp>> responseFutureMap; private BlockingQueue<ShardResponse> responses; + + /** + * The number of pending requests. This must be incremented before a {@link ShardResponse} is + * added to {@link #responses}, and decremented after a ShardResponse is removed from {@code + * responses}. We cannot rely on responses.size() bec Review Comment: missing explanation ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -80,6 +94,39 @@ public HttpShardHandler(HttpShardHandlerFactory httpShardHandlerFactory) { shardToURLs = new HashMap<>(); } + /** + * Parse the {@value ShardParams#SHARDS_TOLERANT} param from <code>params</code> as a boolean; + * accepts {@value ShardParams#REQUIRE_ZK_CONNECTED} as a valid value indicating <code>false + * </code>. + * + * <p>By default, returns <code>false</code> when {@value ShardParams#SHARDS_TOLERANT} is not set + * in <code> + * params</code>. + */ + public static boolean getShardsTolerantAsBool(SolrQueryRequest req) { + String shardsTolerantValue = req.getParams().get(ShardParams.SHARDS_TOLERANT); + if (null == shardsTolerantValue + || shardsTolerantValue.trim().equals(ShardParams.REQUIRE_ZK_CONNECTED)) { + return false; + } else { + boolean tolerant = StrUtils.parseBool(shardsTolerantValue.trim()); + if (tolerant && shouldDiscardPartials(req.getParams())) { Review Comment: this word "discard" is new here (I think) and I find it confusing. Maybe `shouldErrorInsteadOfPartialResults` Or flip the boolean -- allowPartialResults ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -191,7 +244,7 @@ protected QueryRequest makeQueryRequest( } /** Subclasses could modify the Response based on the shard */ - protected ShardResponse transfomResponse( + protected ShardResponse transformResponse( Review Comment: I merged a PR that corrects that this wasn't being called above -- 391cdd04277b83a010f1d33288f9bc344a7ba2d0 ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -139,48 +186,54 @@ 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; - } - - long startTime = System.nanoTime(); - SolrRequestInfo requestInfo = SolrRequestInfo.getRequestInfo(); - if (requestInfo != null) { - req.setUserPrincipal(requestInfo.getReq().getUserPrincipal()); - } + long startTime = System.nanoTime(); Review Comment: please encode time units into time var names, like an "Ns" suffix ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -62,7 +69,14 @@ public class HttpShardHandler extends ShardHandler { private HttpShardHandlerFactory httpShardHandlerFactory; private Map<ShardResponse, CompletableFuture<LBSolrClient.Rsp>> responseFutureMap; Review Comment: It seems responseFutureMap is guarded by RESPONSE_CANCELABLE_LOCK; could use a comment to say so ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java: ########## @@ -216,9 +269,18 @@ public ShardResponse takeCompletedOrError() { private ShardResponse take(boolean bailOnError) { try { + // although nothing in this class guarantees that pending has been incremented to the total + // number of expected requests, actual usage in SearchHandler results in this method never + // being called until all requests have been added in a prior loop over + // ShardRequest.actualShards in the same thread that invokes take() (I haven't checked but + // hopefully other handlers do the same) The net effect is we shouldn't arrive here with Review Comment: Would an assert be useful? ########## solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java: ########## @@ -66,6 +67,22 @@ public static SolrRequestInfo getRequestInfo() { return stack.peek(); } + public static Optional<SolrRequestInfo> getReqInfo() { + return Optional.ofNullable(getRequestInfo()); + } + + public static Optional<SolrQueryRequest> getRequest() { + return getReqInfo().map(i -> i.req); + } + + public static boolean shouldDiscardPartials() { Review Comment: this method feels out of place here ########## solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java: ########## @@ -39,6 +40,36 @@ */ public interface SolrQueryRequest extends AutoCloseable { + /** This is the system property for {@link #ALLOW_PARTIAL_RESULTS_DEFAULT} */ + String SOLR_ALLOW_PARTIAL_RESULTS_DEFAULT = "solr.allowPartialResultsDefault"; + + // silly getBoolean doesn't take a default. + /** + * Users can set {@link SolrQueryRequest#SOLR_ALLOW_PARTIAL_RESULTS_DEFAULT} system property to + * true, and solr will omit results when any shard fails due query execution limits (time, cpu + * etc.). By default, this is set to true. Setting it to false will reduce processing, cpu and + * network associated with collecting and transmitting partial results. This setting can be + * overridden (in either direction) on a per-request basis with {@code + * &allowPartialResults=[true|false]}. When results have been omitted the response header should + * contain a partialResults element with the value "omitted" + */ + boolean ALLOW_PARTIAL_RESULTS_DEFAULT = + System.getProperty(SOLR_ALLOW_PARTIAL_RESULTS_DEFAULT) == null + || Boolean.getBoolean(SOLR_ALLOW_PARTIAL_RESULTS_DEFAULT); Review Comment: EnvUtils -- 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