sigram commented on code in PR #2666:
URL: https://github.com/apache/solr/pull/2666#discussion_r1731349309
##########
solr/CHANGES.txt:
##########
@@ -109,8 +109,11 @@ New Features
Improvements
---------------------
+* SOLR-17158: Users for whom partial results are uninteresting may set
partialResultsAllowed=false. This allows Solr to reduce time spent processing
partial results and omit them from the response. (Gus Heck, Andrzej Bialeki,
hossman)
Review Comment:
Typo - my last name is Bialecki :) or actually Białecki where non-ascii is
allowed, but I've been using both forms.
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -139,30 +142,63 @@ public ReturnFields getReturnFields() {
/**
* If {@link #getResponseHeader()} is available, set {@link
#RESPONSE_HEADER_PARTIAL_RESULTS_KEY}
- * flag to true.
+ * attribute to true or "omitted" as required.
*/
- public void setPartialResults() {
+ public void setPartialResults(SolrQueryRequest req) {
Review Comment:
Should this be `static` now?
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -61,7 +64,7 @@
*/
public class SolrQueryResponse {
public static final String NAME = "response";
- public static final String RESPONSE_HEADER_PARTIAL_RESULTS_KEY =
CommonParams.PARTIAL_RESULTS;
Review Comment:
Originally I did this on purpose to avoid getting these two strings out of
sync.
##########
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc:
##########
@@ -71,6 +71,13 @@ Due to changes in Lucene 9, that isn't possible any more.
=== Configuration
In solrconfig.xml, the `numVersionBuckets` and `versionBucketLockTimeoutMs`
settings are now obsolete and ignored; a warning will be logged if specified.
+=== Partial Results
+When solr is used for reporting, legal discovery or other use cases that
cannot accept partial results, yet it still needs to be protected from
long-running queries, users may now opt out of receiving partial results.
+This feature should eliminate some wasted processing and bandwidth, as well as
simplifying client side logic, and reducing the chance of inappropriate
responses to the user.
+This can be enabled either via a request parameter (`allowPartialResults`),
environment variable or system property.
+The partialResults attribute in the request header will now have three
possible states: `"true"`, `"omitted"` or absent, with the new `"omitted"`
value indicating the difference between a timed out request with results
omitted due to query limits and results that are merely empty because the query
didn't match any docs.
Review Comment:
Somehow this sentence is very confusing to me ... maybe this: the new
"omitted" value indicating that query returned partial results, which due to
the setting were not allowed and they were then discarded? or something more
English-like ;)
##########
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc:
##########
@@ -71,6 +71,13 @@ Due to changes in Lucene 9, that isn't possible any more.
=== Configuration
In solrconfig.xml, the `numVersionBuckets` and `versionBucketLockTimeoutMs`
settings are now obsolete and ignored; a warning will be logged if specified.
+=== Partial Results
+When solr is used for reporting, legal discovery or other use cases that
cannot accept partial results, yet it still needs to be protected from
long-running queries, users may now opt out of receiving partial results.
Review Comment:
long-running queries -> queries exceeding limits ?
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -139,30 +142,63 @@ public ReturnFields getReturnFields() {
/**
* If {@link #getResponseHeader()} is available, set {@link
#RESPONSE_HEADER_PARTIAL_RESULTS_KEY}
- * flag to true.
+ * attribute to true or "omitted" as required.
*/
- public void setPartialResults() {
+ public void setPartialResults(SolrQueryRequest req) {
NamedList<Object> header = getResponseHeader();
- if (header != null
- && header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)
== null) {
- header.add(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY,
Boolean.TRUE);
+ if (header != null) {
+ if (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY) ==
null) {
+ Object value =
partialResultsStatus(shouldDiscardPartials(req.getParams()));
+ header.add(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY,
value);
+ }
}
}
+ public static Object partialResultsStatus(boolean discarding) {
+ return discarding ? "omitted" : Boolean.TRUE;
Review Comment:
Should this be a constant 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: 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
