sigram commented on code in PR #2801:
URL: https://github.com/apache/solr/pull/2801#discussion_r1834075014
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -626,44 +697,97 @@ 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();
- }
+ private static boolean prepareComponents(
+ SolrQueryRequest req, ResponseBuilder rb, RTimerTree timer,
List<SearchComponent> components)
+ throws IOException {
+ if (timer == null) {
+ // non-debugging prepare phase
+ for (SearchComponent component : components) {
+ if (checkLimitsBefore(component, "prepare", rb.req, rb.rsp,
components)) {
+ shortCircuitedResults(req, rb);
+ return false;
}
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
+ component.prepare(rb);
+ }
+ } else {
+ // debugging prepare phase
+ RTimerTree subt = timer.sub("prepare");
+ for (SearchComponent c : components) {
+ if (checkLimitsBefore(c, "prepare debug", rb.req, rb.rsp, components))
{
+ shortCircuitedResults(req, rb);
+ return false;
}
- } 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());
+ rb.setTimer(subt.sub(c.getName()));
+ c.prepare(rb);
+ rb.getTimer().stop();
}
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ subt.stop();
+ }
+ return true;
+ }
- 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 String stageInEnglish(int nextStage) {
+ // This should probably be a enum, but that change should be its own
ticket.
+ 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);
+ }
+ }
+
+ 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);
+ }
+ rb.rsp.setPartialResults(rb.req);
+ }
+
+ private static boolean checkLimitsBefore(
+ SearchComponent c,
+ String when,
+ SolrQueryRequest req,
+ SolrQueryResponse resp,
+ List<SearchComponent> components) {
+
+ return getQueryLimits(req, resp)
+ .maybeExitWithPartialResults(
+ () ->
+ "["
+ + when
+ + "] Limit(s) exceeded prior to "
+ + c.getName()
+ + " in "
+ + components.stream()
+ .map(SearchComponent::getName)
+ .collect(Collectors.toList()));
Review Comment:
Maybe we should build a list of all component names in `initComponents()`
and keep it around?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -306,9 +306,12 @@ public synchronized Endpoint nextOrError(Exception
previousEx) throws SolrServer
suffix = ":" + zombieServers.keySet();
}
// Skipping check time exceeded for the first request
- if (numServersTried > 0 && isTimeExceeded(timeAllowedNano, timeOutTime))
{
+ // Ugly string based hack but no live servers message here is VERY
misleading :(
+ if ((previousEx != null && previousEx.getMessage().contains("Limits
exceeded!"))
+ || (numServersTried > 0 && isTimeExceeded(timeAllowedNano,
timeOutTime))) {
throw new SolrServerException(
- "Time allowed to handle this request exceeded" + suffix,
previousEx);
+ "The processing limits for to this request were exceeded, see
cause for details",
Review Comment:
Yes!
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -615,17 +615,23 @@ protected int groupedDistributedProcess(ResponseBuilder
rb) {
}
protected int regularDistributedProcess(ResponseBuilder rb) {
- if (rb.stage < ResponseBuilder.STAGE_PARSE_QUERY) return
ResponseBuilder.STAGE_PARSE_QUERY;
+ if (rb.stage < ResponseBuilder.STAGE_PARSE_QUERY) {
Review Comment:
+1, I never liked these one-liners.
##########
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:
Maybe use `CallerSpecificQueryLimit` (example in
`TestQueryLimits.testQueryLimits()`.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -626,44 +697,97 @@ 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();
- }
+ private static boolean prepareComponents(
+ SolrQueryRequest req, ResponseBuilder rb, RTimerTree timer,
List<SearchComponent> components)
+ throws IOException {
+ if (timer == null) {
+ // non-debugging prepare phase
+ for (SearchComponent component : components) {
+ if (checkLimitsBefore(component, "prepare", rb.req, rb.rsp,
components)) {
+ shortCircuitedResults(req, rb);
+ return false;
}
- nl.add("error", cause.toString());
- if (!core.getCoreContainer().hideStackTrace()) {
- StringWriter trace = new StringWriter();
- cause.printStackTrace(new PrintWriter(trace));
- nl.add("trace", trace.toString());
+ component.prepare(rb);
+ }
+ } else {
+ // debugging prepare phase
+ RTimerTree subt = timer.sub("prepare");
+ for (SearchComponent c : components) {
+ if (checkLimitsBefore(c, "prepare debug", rb.req, rb.rsp, components))
{
+ shortCircuitedResults(req, rb);
+ return false;
}
- } 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());
+ rb.setTimer(subt.sub(c.getName()));
+ c.prepare(rb);
+ rb.getTimer().stop();
}
- nl.add("shardAddress", rb.shortCircuitedURL);
- nl.add("time", req.getRequestTimer().getTime()); // elapsed time of this
request so far
+ subt.stop();
+ }
+ return true;
+ }
- 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 String stageInEnglish(int nextStage) {
+ // This should probably be a enum, but that change should be its own
ticket.
Review Comment:
+1 to both.
--
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
