sigram commented on code in PR #2403:
URL: https://github.com/apache/solr/pull/2403#discussion_r1566013026
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -621,46 +684,72 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rsp.addToLog(ThreadCpuTimer.CPU_TIME, totalShardCpuTime);
}
}
+ }
- // SOLR-5550: still provide shards.info if requested even for a short
circuited distrib request
- if (!rb.isDistrib
- && req.getParams().getBool(ShardParams.SHARDS_INFO, false)
- && rb.shortCircuitedURL != null) {
- NamedList<Object> shardInfo = new SimpleOrderedMap<>();
- SimpleOrderedMap<Object> nl = new SimpleOrderedMap<>();
- if (rsp.getException() != null) {
- Throwable cause = rsp.getException();
- if (cause instanceof SolrServerException) {
- cause = ((SolrServerException) cause).getRootCause();
- } else {
- if (cause.getCause() != null) {
- cause = cause.getCause();
- }
- }
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
- }
- } else if (rb.getResults() != null) {
- nl.add("numFound", rb.getResults().docList.matches());
- nl.add(
- "numFoundExact",
- rb.getResults().docList.hitCountRelation() ==
TotalHits.Relation.EQUAL_TO);
- nl.add("maxScore", rb.getResults().docList.maxScore());
- }
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ private static String stageInEngilish(int nextStage) {
Review Comment:
Engilish -> English :)
##########
solr/core/src/java/org/apache/solr/search/QueryLimits.java:
##########
@@ -108,12 +110,21 @@ public String formatExceptionMessage(String label) {
* @throws QueryLimitsExceededException if {@link
CommonParams#PARTIAL_RESULTS} request parameter
* is false and limits have been reached.
*/
+ public boolean maybeExitWithPartialResults(Supplier<String> label)
+ throws QueryLimitsExceededException {
+ return maybeExitWithPartialResults(label.get());
+ }
+
public boolean maybeExitWithPartialResults(String label) throws
QueryLimitsExceededException {
if (isLimitsEnabled() && shouldExit()) {
if (allowPartialResults) {
if (rsp != null) {
rsp.setPartialResults();
- rsp.addPartialResponseDetail(formatExceptionMessage(label));
+ if
(rsp.getResponseHeader().get(RESPONSE_HEADER_PARTIAL_RESULTS_DETAILS_KEY) ==
null) {
Review Comment:
This loses information about the exceeded limits inside some components,
when we accept partial results (ie. exception is not thrown immediately). Why
wouldn't duplicate keys be ok? NamedList is essentially a multi-map so multiple
keys of the same value are explicitly allowed.
##########
solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java:
##########
@@ -70,9 +70,12 @@ public void testPrefixQuery() throws Exception {
// this time we should get a query cache hit and hopefully no exception?
this may change in the
// future if time checks are put into other places.
- assertJQ(req("q", q, "timeAllowed", "1", "sleep", sleep), assertionString);
+ // 2024-4-15: it did change..., and now this fails with 1 or 2 ms and
passes with 3ms... I see
+ // no way this won't be terribly brittle. Maybe TestInjection of some sort
to bring this back?
Review Comment:
Definitely the answer :) please see how it's used in `TestQueryLimits`.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -621,46 +684,72 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rsp.addToLog(ThreadCpuTimer.CPU_TIME, totalShardCpuTime);
}
}
+ }
- // SOLR-5550: still provide shards.info if requested even for a short
circuited distrib request
- if (!rb.isDistrib
- && req.getParams().getBool(ShardParams.SHARDS_INFO, false)
- && rb.shortCircuitedURL != null) {
- NamedList<Object> shardInfo = new SimpleOrderedMap<>();
- SimpleOrderedMap<Object> nl = new SimpleOrderedMap<>();
- if (rsp.getException() != null) {
- Throwable cause = rsp.getException();
- if (cause instanceof SolrServerException) {
- cause = ((SolrServerException) cause).getRootCause();
- } else {
- if (cause.getCause() != null) {
- cause = cause.getCause();
- }
- }
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
- }
- } else if (rb.getResults() != null) {
- nl.add("numFound", rb.getResults().docList.matches());
- nl.add(
- "numFoundExact",
- rb.getResults().docList.hitCountRelation() ==
TotalHits.Relation.EQUAL_TO);
- nl.add("maxScore", rb.getResults().docList.maxScore());
- }
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ private static String stageInEngilish(int nextStage) {
+ // This should probably be a enum, but that change should be its own
ticket.
Review Comment:
+1 for enum (in a separate PR)
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -621,46 +684,72 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rsp.addToLog(ThreadCpuTimer.CPU_TIME, totalShardCpuTime);
}
}
+ }
- // SOLR-5550: still provide shards.info if requested even for a short
circuited distrib request
- if (!rb.isDistrib
- && req.getParams().getBool(ShardParams.SHARDS_INFO, false)
- && rb.shortCircuitedURL != null) {
- NamedList<Object> shardInfo = new SimpleOrderedMap<>();
- SimpleOrderedMap<Object> nl = new SimpleOrderedMap<>();
- if (rsp.getException() != null) {
- Throwable cause = rsp.getException();
- if (cause instanceof SolrServerException) {
- cause = ((SolrServerException) cause).getRootCause();
- } else {
- if (cause.getCause() != null) {
- cause = cause.getCause();
- }
- }
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
- }
- } else if (rb.getResults() != null) {
- nl.add("numFound", rb.getResults().docList.matches());
- nl.add(
- "numFoundExact",
- rb.getResults().docList.hitCountRelation() ==
TotalHits.Relation.EQUAL_TO);
- nl.add("maxScore", rb.getResults().docList.maxScore());
- }
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ private static String stageInEngilish(int nextStage) {
+ // This should probably be a enum, but that change should be its own
ticket.
+ // public static int STAGE_START = 0;
+ //
+ // public static int STAGE_PARSE_QUERY = 1000;
+ // public static int STAGE_TOP_GROUPS = 1500;
+ // public static int STAGE_EXECUTE_QUERY = 2000;
+ // public static int STAGE_GET_FIELDS = 3000;
+ // public static int STAGE_DONE = Integer.MAX_VALUE;
+ switch (nextStage) {
+ case STAGE_START:
+ return "START";
+ case STAGE_PARSE_QUERY:
+ return "PARSE_QUERY";
+ case STAGE_TOP_GROUPS:
+ return "TOP_GROUPS";
+ case STAGE_EXECUTE_QUERY:
+ return "EXECUTE_QUERY";
+ case STAGE_GET_FIELDS:
+ return "GET_FIELDS";
+ // nobody wants to think it was DONE and canceled after it completed...
+ case STAGE_DONE:
+ return "FINISHING";
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unrecognized stage:" +
nextStage);
+ }
+ }
- int pos = rb.shortCircuitedURL.indexOf("://");
- String shardInfoName =
- pos != -1 ? rb.shortCircuitedURL.substring(pos + 3) :
rb.shortCircuitedURL;
- shardInfo.add(shardInfoName, nl);
- rsp.getValues().add(ShardParams.SHARDS_INFO, shardInfo);
+ private static void shortCircuitedResults(SolrQueryRequest req,
ResponseBuilder rb) {
+
+ if (rb.rsp.getResponse() == null) {
+ rb.rsp.addResponse(new SolrDocumentList());
+
+ // If a cursorMark was passed, and we didn't progress, set
+ // the nextCursorMark to the same position
+ String cursorStr =
rb.req.getParams().get(CursorMarkParams.CURSOR_MARK_PARAM);
+ if (null != cursorStr) {
+ rb.rsp.add(CursorMarkParams.CURSOR_MARK_NEXT, cursorStr);
+ }
+ }
+ if (rb.isDebug()) {
+ NamedList<Object> debug = new NamedList<>();
+ debug.add("explain", new NamedList<>());
+ rb.rsp.add("debug", debug);
}
}
+ private static boolean checkLimitsBefore(
+ SearchComponent c,
+ String when,
Review Comment:
Hmm, this name looks confusing ... maybe `stageName` or `stageLabel` ?
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -422,18 +431,74 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
return; // Circuit breaker tripped, return immediately
}
+ processComponents(req, rsp, rb, timer, components);
+
+ // SOLR-5550: still provide shards.info if requested even for a
short-circuited distrib request
+ if (!rb.isDistrib
+ && req.getParams().getBool(ShardParams.SHARDS_INFO, false)
+ && rb.shortCircuitedURL != null) {
+ NamedList<Object> shardInfo = new SimpleOrderedMap<>();
+ SimpleOrderedMap<Object> nl = new SimpleOrderedMap<>();
+ if (rsp.getException() != null) {
+ Throwable cause = rsp.getException();
+ if (cause instanceof SolrServerException) {
+ cause = ((SolrServerException) cause).getRootCause();
+ } else {
+ if (cause.getCause() != null) {
+ cause = cause.getCause();
+ }
+ }
+ nl.add("error", cause.toString());
+ if (!core.getCoreContainer().hideStackTrace()) {
+ StringWriter trace = new StringWriter();
+ cause.printStackTrace(new PrintWriter(trace));
+ nl.add("trace", trace.toString());
+ }
+ } else if (rb.getResults() != null) {
+ nl.add("numFound", rb.getResults().docList.matches());
+ nl.add(
+ "numFoundExact",
+ rb.getResults().docList.hitCountRelation() ==
TotalHits.Relation.EQUAL_TO);
+ nl.add("maxScore", rb.getResults().docList.maxScore());
+ }
+ nl.add("shardAddress", rb.shortCircuitedURL);
+ nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+
+ int pos = rb.shortCircuitedURL.indexOf("://");
+ String shardInfoName =
+ pos != -1 ? rb.shortCircuitedURL.substring(pos + 3) :
rb.shortCircuitedURL;
+ shardInfo.add(shardInfoName, nl);
+ rsp.getValues().add(ShardParams.SHARDS_INFO, shardInfo);
+ }
+ }
+
+ private void processComponents(
+ SolrQueryRequest req,
+ SolrQueryResponse rsp,
+ ResponseBuilder rb,
+ RTimerTree timer,
+ List<SearchComponent> components)
+ throws IOException {
// creates a ShardHandler object only if it's needed
final ShardHandler shardHandler1 = getAndPrepShardHandler(req, rb);
if (timer == null) {
// non-debugging prepare phase
for (SearchComponent c : components) {
+ if (checkLimitsBefore(c, "prepare", rb.req, rb.rsp, components)) {
Review Comment:
Nice! that's very useful info for troubleshooting.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -621,46 +684,72 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rsp.addToLog(ThreadCpuTimer.CPU_TIME, totalShardCpuTime);
}
}
+ }
- // SOLR-5550: still provide shards.info if requested even for a short
circuited distrib request
- if (!rb.isDistrib
- && req.getParams().getBool(ShardParams.SHARDS_INFO, false)
- && rb.shortCircuitedURL != null) {
- NamedList<Object> shardInfo = new SimpleOrderedMap<>();
- SimpleOrderedMap<Object> nl = new SimpleOrderedMap<>();
- if (rsp.getException() != null) {
- Throwable cause = rsp.getException();
- if (cause instanceof SolrServerException) {
- cause = ((SolrServerException) cause).getRootCause();
- } else {
- if (cause.getCause() != null) {
- cause = cause.getCause();
- }
- }
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
- }
- } else if (rb.getResults() != null) {
- nl.add("numFound", rb.getResults().docList.matches());
- nl.add(
- "numFoundExact",
- rb.getResults().docList.hitCountRelation() ==
TotalHits.Relation.EQUAL_TO);
- nl.add("maxScore", rb.getResults().docList.maxScore());
- }
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ private static String stageInEngilish(int nextStage) {
+ // This should probably be a enum, but that change should be its own
ticket.
+ // public static int STAGE_START = 0;
+ //
+ // public static int STAGE_PARSE_QUERY = 1000;
+ // public static int STAGE_TOP_GROUPS = 1500;
+ // public static int STAGE_EXECUTE_QUERY = 2000;
+ // public static int STAGE_GET_FIELDS = 3000;
+ // public static int STAGE_DONE = Integer.MAX_VALUE;
+ switch (nextStage) {
+ case STAGE_START:
+ return "START";
+ case STAGE_PARSE_QUERY:
+ return "PARSE_QUERY";
+ case STAGE_TOP_GROUPS:
+ return "TOP_GROUPS";
+ case STAGE_EXECUTE_QUERY:
+ return "EXECUTE_QUERY";
+ case STAGE_GET_FIELDS:
+ return "GET_FIELDS";
+ // nobody wants to think it was DONE and canceled after it completed...
+ case STAGE_DONE:
+ return "FINISHING";
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unrecognized stage:" +
nextStage);
+ }
+ }
- int pos = rb.shortCircuitedURL.indexOf("://");
- String shardInfoName =
- pos != -1 ? rb.shortCircuitedURL.substring(pos + 3) :
rb.shortCircuitedURL;
- shardInfo.add(shardInfoName, nl);
- rsp.getValues().add(ShardParams.SHARDS_INFO, shardInfo);
+ private static void shortCircuitedResults(SolrQueryRequest req,
ResponseBuilder rb) {
+
+ if (rb.rsp.getResponse() == null) {
+ rb.rsp.addResponse(new SolrDocumentList());
+
+ // If a cursorMark was passed, and we didn't progress, set
+ // the nextCursorMark to the same position
+ String cursorStr =
rb.req.getParams().get(CursorMarkParams.CURSOR_MARK_PARAM);
+ if (null != cursorStr) {
+ rb.rsp.add(CursorMarkParams.CURSOR_MARK_NEXT, cursorStr);
+ }
+ }
+ if (rb.isDebug()) {
+ NamedList<Object> debug = new NamedList<>();
+ debug.add("explain", new NamedList<>());
+ rb.rsp.add("debug", debug);
}
}
+ private static boolean checkLimitsBefore(
+ SearchComponent c,
+ String when,
+ SolrQueryRequest req,
+ SolrQueryResponse resp,
+ List<SearchComponent> components) {
+
+ return getQueryLimits(req, resp)
+ .maybeExitWithPartialResults(
+ () -> {
Review Comment:
This lambda is evaluated in every call to `checkLimitsBefore`, which seems
excessive. Maybe we could cache the list of component names somewhere?
--
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]
