[
https://issues.apache.org/jira/browse/SOLR-17172?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17820226#comment-17820226
]
Chris M. Hostetter commented on SOLR-17172:
-------------------------------------------
{quote}Chris M. Hostetter had identified that my changes in recent 9.x (to
account for Lucene changes) had the down side that some code that touches the
index would no longer be governed by the timeAllowed. ... but I suspect the
ideal fix should be to find a way to incorporate checks when accessing the
index (i.e. at a lower level), as it was done with ExitableDirectoryReader.
{quote}
The crux of my concern in that conversation (SOLR-16693) boiled down to...
{quote}"Are we there yet?" type checks in the solr level will never be as good
as ExitableDirectoryReader.
{quote}
...and i still stand by that, but that doesn't mean I disagree with adding
checks like that at the Solr level (Sorry if i gave you that impression David).
I don't think we need {{QueryLimits.shouldExit()}} checks on every line of Solr
code, but I agree with AB that it makes sense to include these types of checks
in Solr components to break out of potentially expensive Solr loops – in the
same way ExitableDirectoryReader helps us break out of potentially expensive
Lucene loops.
----
As far as the current PR goes...
* I like the new {{public static QueryLimits getCurrentLimits()}} method,
but...
** There are a handful a places in the code base that are already modified by
this patch, but instead of using the new {{QueryLimits getCurrentLimits()}}
helper they still go get the {{SolrRequestInfo}} for the sole purpose of
checking if {{QueryLimits}} are in use – they should really be updated to use
this method as well.
** A couple are in {{SolrIndexSearcher}}
** One in {{SpellCheckComponent.prepare(...)}}
* Since the {{QueryLimits}} constructor already takes in the
{{SolrQueryRequest}} object, it seems like {{QueryLimits}} instance it should
keep track of a {{final boolean partialResultsOk}} that it can initialize in
it's constructor based on
{{req.getParams().getBool(CommonParams.PARTIAL_RESULTS, true)}}
** That would eliminate the need for {{maybeExitWithPartialResults(...)}} to
check for {{CommonParams.PARTIAL_RESULTS}} on every call – it can just check
that final boolean.
*** Which would also eliminate the need to pass {{SolrQueryRequest}} as an
argument to {{maybeExitWithPartialResults(...)}}
** And I can't help but wonder if the {{QueryLimits}} constructor shouldn't
also take in a {{SolrQueryResponse}} that it keeps around, so that
{{maybeExitWithPartialResults(...)}} won't need to require that argument either?
*** The {{SolrQueryResponse}} is already available when the {{QueryLimits}} is
constructed by {{SolrRequestInfo.}}
* The logic in {{maybeExitWithPartialResults(...)}} for modifying the response
to set {{RESPONSE_HEADER_PARTIAL_RESULTS_KEY}} & add the
{{RESPONSE_HEADER_PARTIAL_RESULTS_DETAILS_KEY}} seems like something that
should probably be refactored into a new method in {{SolrQueryResponse}} ?
** And the handful of other existing code paths that currently set
{{RESPONSE_HEADER_PARTIAL_RESULTS_KEY}} on the response should be updated to
use this method as well ?
* The call to {{maybeExitWithPartialResults(...)}} inside
{{FacetComponent.finishStage()}} seems like it might be better after the
{{rb._facetInfo = null;}} line?
* We still need ref-guide updates for the new {{partialResults}} request param
to explain how it changes the behavior when limits are used.
> Add QueryLimits termination to existing heavy SearchComponent-s
> ---------------------------------------------------------------
>
> Key: SOLR-17172
> URL: https://issues.apache.org/jira/browse/SOLR-17172
> Project: Solr
> Issue Type: Sub-task
> Reporter: Andrzej Bialecki
> Assignee: Andrzej Bialecki
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The purpose of this ticket is to review the existing {{SearchComponent}}-s
> that perform intensive tasks to see if they could be modified to check the
> {{QueryLimits.shouldExit()}} inside their execution.
> This is not meant to be included in tight loops but to prevent individual
> components from completing multiple stages of costly work that will be
> discarded anyway on the exit from the component due to the exceeded limits
> (SOLR-17151).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
