lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anshum Gupta" <ans...@apache.org>
Subject Re: Review Request 25658: Timeout queries when they take too long to rewrite/enumerate over terms.
Date Wed, 17 Sep 2014 02:31:31 GMT


> On Sept. 17, 2014, 1:48 a.m., Robert Muir wrote:
> >

Thanks for taking a look at this Robert!


> On Sept. 17, 2014, 1:48 a.m., Robert Muir wrote:
> > trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java,
line 46
> > <https://reviews.apache.org/r/25658/diff/3/?file=691074#file691074line46>
> >
> >     Can we somehow share this with TimeLimitingCollector.TimeExceededException?

That's certainly on my radar but would want to have a new issue for that. The TimeExceededException
includes things that wouldn't make sense while iterating over TermsEnum as it's supposed to
be thrown during Collection.


> On Sept. 17, 2014, 1:48 a.m., Robert Muir wrote:
> > trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java,
line 80
> > <https://reviews.apache.org/r/25658/diff/3/?file=691074#file691074line80>
> >
> >     Thanks for removing the redundant methods! However, because this timeout doesnt
actually change results, we should probably override the two cache methods getCoreCacheKey()/getCombinedCoreAndDeletesKey()
to explicitly call super? Otherwise anything using this won't really support NRT at all. I
think these are not handled by FilterAtomicReader by default. The latter should of course
really be removed, as it makes no sense, but thats for another issue :)

Sure, made that change, the next patch shall contain it.


> On Sept. 17, 2014, 1:48 a.m., Robert Muir wrote:
> > trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java,
line 162
> > <https://reviews.apache.org/r/25658/diff/3/?file=691074#file691074line162>
> >
> >     Is it possible for the exceptions thrown from this to contain more detailed
information? For example, the timeout could contain the timeout value, and maybe also contain
"terms=" + this in the exception message, to give a little more context as to where it happened.

+1 on that. Made that change.


> On Sept. 17, 2014, 1:48 a.m., Robert Muir wrote:
> > trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeout.java, line 27
> > <https://reviews.apache.org/r/25658/diff/3/?file=691075#file691075line27>
> >
> >     Do we need this concrete class + QueryTimeOutBase or can we simplify into one
thing, maybe just a one-method interface? For java 8 especially this should work pretty intuitively.
Just an idea.

Until we're on 8, I'd say, let's go with this. The reason I did it that way was to keep the
ThreadLocal implementation run parallel for Solr.


> On Sept. 17, 2014, 1:48 a.m., Robert Muir wrote:
> > trunk/lucene/core/src/test/org/apache/lucene/index/TestExitableDirectoryReader.java,
line 137
> > <https://reviews.apache.org/r/25658/diff/3/?file=691077#file691077line137>
> >
> >     Again I'm concerned about a wall-clock time in tests. If we already have the
way to override the guy going into this, can't we just override it to throw exception e.g.
on the n'th time its invoked or something so the tests are stable?

The timeout is 1ms, the sleep is for 1000ms so I don't really see a reason why this would
fail. Theoretically, it shouldn't.
Also, I'm trying to test out TimingOut so I'd ideally want to hit the timeout and exit instead
of throw the Exception from the TestTermsEnum wrapper.


- Anshum


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/#review53638
-----------------------------------------------------------


On Sept. 16, 2014, 9:40 p.m., Anshum Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25658/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 9:40 p.m.)
> 
> 
> Review request for lucene.
> 
> 
> Bugs: SOLR-5986
>     https://issues.apache.org/jira/browse/SOLR-5986
> 
> 
> Repository: lucene
> 
> 
> Description
> -------
> 
> Timeout queries when they take too long to rewrite/enumerate over terms.
> 
> 
> Diffs
> -----
> 
>   trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java PRE-CREATION

>   trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeout.java PRE-CREATION 
>   trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeoutBase.java PRE-CREATION

>   trunk/lucene/core/src/test/org/apache/lucene/index/TestExitableDirectoryReader.java
PRE-CREATION 
>   trunk/solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java 1625349 
>   trunk/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java 1625349

>   trunk/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java 1625349 
>   trunk/solr/core/src/java/org/apache/solr/search/SolrQueryTimeout.java PRE-CREATION

>   trunk/solr/core/src/test/org/apache/solr/TestDistributedSearch.java 1625349 
>   trunk/solr/core/src/test/org/apache/solr/TestGroupingSearch.java 1625349 
>   trunk/solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/25658/diff/
> 
> 
> Testing
> -------
> 
> Added Lucene/Solr tests. Tested a bit manually.
> 
> 
> Thanks,
> 
> Anshum Gupta
> 
>


Mime
View raw message