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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
