SOLR-17151 - stop processing components once we have exceeded a query limit #2403
Conversation
|
Still needs unit test... |
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
Outdated
Show resolved
Hide resolved
| 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. |
There was a problem hiding this comment.
+1 for enum (in a separate PR)
| 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) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Well typically before I did this we would get the initial limit violation, and then at least one component violation which isn't really interesting because the first one is where we really stopped. Also there are tests that check the contents of partialResultsDetail using JSONTestUtil.match() which doesn't seem to have a syntax for handling duplicate keys (or if it does I can't figure out what it is), and also if someone out there has been using a Jackson Object Mapper or similar to turn the header into an object (or JSON.parse() in javascript where they'll loose one anyway) things could go sideways if they suddenly encounter either a duplicate key or an array there... I can provide a rant about the fact we allow/use duplicate keys in the first place, how we should be referring to our APIs as noggit APIs not JSON APIs because we accept illegal JSON... etc... If you're interested :)
There was a problem hiding this comment.
Hmm, ok... then maybe we should add some processing similar to computeShardCpuTime to aggregate multiple details from shard responses into a single value?
| @@ -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? | |||
There was a problem hiding this comment.
Definitely the answer :) please see how it's used in TestQueryLimits.
| // 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)) { |
There was a problem hiding this comment.
Nice! that's very useful info for troubleshooting.
| } | ||
| } | ||
|
|
||
| private static boolean checkLimitsBefore( | ||
| SearchComponent c, | ||
| String when, |
There was a problem hiding this comment.
Hmm, this name looks confusing ... maybe stageName or stageLabel ?
There was a problem hiding this comment.
stage is what we use for the finishing steps... maybe phase?
| } | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| public boolean maybeExitWithPartialResults(String label) throws QueryLimitsExceededException { |
There was a problem hiding this comment.
I think we should add some javadoc here that explains why we have two different methods for doing essentially the same work, and when to prefer one over the other.
|
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the [email protected] mailing list. Thank you for your contribution! |
|
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. |
https://issues.apache.org/jira/browse/SOLR-17151