dsmiley commented on code in PR #2290:
URL: https://github.com/apache/solr/pull/2290#discussion_r1503209868
##########
solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java:
##########
@@ -164,6 +165,8 @@ public void process(ResponseBuilder rb) throws IOException {
// TODO ???? add this directly to the response?
rb.rsp.add(highlightingResponseField(), convertHighlights(sumData));
}
+ QueryLimits queryLimits = QueryLimits.getCurrentLimits();
+ queryLimits.maybeExitWithPartialResults("Highlighting process");
Review Comment:
Don't we only want to do these checks in expensive loops, basically?
There's no loop here.
I suggest considering adding to SearchHandler where it loops the components
to call process() and potentially somoe other methods. This would broadly
cover components in a course sense; I think it would alleviate your desire to
put the check where you did here in highlighting (and maybe some other
components).
##########
solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc:
##########
@@ -284,6 +284,17 @@ The query above allows you to examine the scoring explain
info of the top matchi
The default value of this parameter is blank, which causes no extra "explain
info" to be returned.
+== partialResults Parameter
+
+This parameter controls Solr's behavior when a query execution limit is
reached (e.g. `timeAllowed` or `cpuAllowed`).
+
+When this parameter is set to `true` (default) then even though reaching a
limit terminates further query processing
+Solr will still attempt to return partial results collected so far. These
results may be incomplete in
Review Comment:
In our ascii-doc files, generally start new sentences on new lines. Easier
to read diffs in the future.
Also this sentence is missing an "a" before "non-deterministic"
##########
solr/core/src/java/org/apache/solr/search/QueryLimits.java:
##########
@@ -73,27 +136,32 @@ public boolean shouldExit() {
* @return A string describing the state pass/fail state of each limit
specified for this request.
*/
public String limitStatusMessage() {
- StringBuilder sb = new StringBuilder();
- boolean first = true;
+ if (limits.isEmpty()) {
+ return "This request is unlimited.";
+ }
+ StringBuilder sb = new StringBuilder("Query limits: ");
for (QueryTimeout limit : limits) {
- if (first) {
- first = false;
- sb.append("Query limits:");
- }
sb.append("[");
sb.append(limit.getClass().getSimpleName());
sb.append(":");
sb.append(limit.shouldExit() ? "LIMIT EXCEEDED" : "within limit");
sb.append("]");
}
- if (sb.length() == 0) {
- return "This request is unlimited.";
- } else {
- return sb.toString();
- }
+ return sb.toString();
}
- public boolean isTimeoutEnabled() {
+ /** Return true if there are any limits enabled for the current request. */
+ public boolean isLimitsEnabled() {
return !limits.isEmpty();
}
+
+ /**
+ * Helper method to retrieve the current QueryLimits from {@link
SolrRequestInfo#getRequestInfo()}
+ * if it exists, otherwise it returns {@link #NONE}.
+ */
+ public static QueryLimits getCurrentLimits() {
+ return SolrRequestInfo.getRequestInfo() != null
Review Comment:
Can you store this into a variable and then do the null check? Super minor
I know but let's not do a redundant ThreadLocal lookup.
##########
solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java:
##########
Review Comment:
ExpandComponent doesn't do heavy work; I don't see the point
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -204,10 +204,7 @@ private static DirectoryReader wrapReader(SolrCore core,
DirectoryReader reader)
assert reader != null;
reader = UninvertingReader.wrap(reader,
core.getLatestSchema().getUninversionMapper());
if (useExitableDirectoryReader) { // SOLR-16693 legacy; may be removed.
Probably inefficient.
- SolrRequestInfo requestInfo = SolrRequestInfo.getRequestInfo();
- assert requestInfo != null;
- QueryLimits limits = requestInfo.getLimits();
- reader = ExitableDirectoryReader.wrap(reader, limits);
+ reader = ExitableDirectoryReader.wrap(reader,
QueryLimits.getCurrentLimits());
Review Comment:
Way better; thank you.
##########
solr/core/src/java/org/apache/solr/handler/component/MoreLikeThisComponent.java:
##########
@@ -127,6 +128,8 @@ public void process(ResponseBuilder rb) throws IOException {
getMoreLikeThese(rb, rb.req.getSearcher(),
rb.getResults().docList, flags);
rb.rsp.add("moreLikeThis", sim);
}
+ QueryLimits queryLimits = QueryLimits.getCurrentLimits();
+ queryLimits.maybeExitWithPartialResults("MoreLikeThis process");
Review Comment:
hard to tell but is this in the per-doc loop? If it is, cool. If it isn't;
this is dubious.
--
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]
