> 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 > >
