SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer#1450
SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer#1450dsmiley merged 5 commits intoapache:mainfrom
Conversation
instead of ExitableDirectoryReader
dsmiley
requested review from
HoustonPutman,
bruno-roustant,
cpoerschke and
uschindler
| throws IOException { | ||
| final var queryTimeout = SolrQueryTimeoutImpl.getInstance(); | ||
| if (queryTimeout.isTimeoutEnabled() == false) { | ||
| // no timeout. Pass through to subclass |
There was a problem hiding this comment.
| // no timeout. Pass through to subclass | |
| // no timeout. Pass through to super class |
| throw new ExitableDirectoryReader.ExitingReaderException( | ||
| "Timed out. Not actually via ExitableDirectoryReader"); |
There was a problem hiding this comment.
Maybe an alternative might be to have a class SolrNotActuallyExitableDirectoryReader extends ExitableDirectoryReader.ExitingReaderException approach which would be thrown here and then over time the various places could be switched over to check for SolrNotActuallyExitableDirectoryReader instead or to have the checks phased out.
There was a problem hiding this comment.
I assume you're not serious about the name SolrNotActuallyExitableDirectoryReader. Two name proposals:
- TimeAllowedExceededException -- generic. But despite it being generic, the reality is that there are other exceptions that are effectively similar that would still exist, so maybe give up on trying to be generic?
- TimeAllowedExceededFromScorerException -- more specific as to what makes this timeout what it is.
There was a problem hiding this comment.
I assume you're not serious about the name SolrNotActuallyExitableDirectoryReader. Two name proposals: ...
Yeah, that was not meant to be a serious name suggestion but only a way to convey the class ... extends ExitableDirectoryReader.ExitingReaderException idea.
TimeAllowedExceededException vs. TimeAllowedExceededFromScorerException -- the latter to me seems closer to the current ExitableDirectoryReader.ExitingReaderException which is also pretty specific.
There was a problem hiding this comment.
Good; I chose the latter; PR updated.
I think it'd be worthwhile for Lucene to have a base class of these exceptions that is public, therefore Lucene apps can catch one general exception no matter which approach is being taken.
|
BTW I won't merge this any sooner than the Lucene 9.5 PR merges, so that these both can ultimately ship on the same release. Proposed CHANGES.txt under improvements:
|
|
I plan to merge end of Monday or so. |
| // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention | ||
| // that timedOut() might race with concurrent queries (dubious design). | ||
| // So we need to make a new IndexSearcher instead of using "this". | ||
| new IndexSearcher(reader) { // cheap, actually! |
There was a problem hiding this comment.
Maybe we should open up a Lucene issue that gives us a search(...., timeout) method, so that this hack isn't necessary in the future. Though as you mention, that would include adding a similar TimeAllowedExceeded Exception to Lucene.
For timeAllowed: use more efficient TimeLimitingBulkScorer instead of ExitableDirectoryReader Added sys prop escape hatch: solr.useExitableDirectoryReader
4 participants
Instead of ExitableDirectoryReader
https://issues.apache.org/jira/browse/SOLR-16693
TODO... maybe allow use of ExitableDirectoryReader via sys prop? BTW there are code paths using the reader that wouldn't use the search method yet may have a timeAllowed param. I think I've seen this in an erroneous (bug or accident; haven't root caused yet) scenario at work but could be valid.
Wish we had benchmarks on this. Any way, it's interesting that all tests pass so easily :-)