> On Sept. 17, 2014, 3:44 a.m., Steve Davids wrote: > > trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeout.java, line > > 56 > > <https://reviews.apache.org/r/25658/diff/4/?file=691686#file691686line56> > > > > You can provide the shouldExit implementation in the abstract class if > > you make the getTimeoutAt() abstract.
shouldExit() is more intuitive for users to implement when they want their own custom logic for exiting the queries. > On Sept. 17, 2014, 3:44 a.m., Steve Davids wrote: > > trunk/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java, > > line 258 > > <https://reviews.apache.org/r/25658/diff/4/?file=691690#file691690line258> > > > > Is the reset necessary? Would it make sense to just start the > > SolrQueryTimeout clock right when a request is being serviced and let it > > run until the ThreadLocal is eventually destroyed? the reset is necessary for the explicit removal of the ThreadLocal value. ThreadLocal doesn't clear itself implicitly up after the lifecycle of the thread and so the reset is more than necessary. > On Sept. 17, 2014, 3:44 a.m., Steve Davids wrote: > > trunk/solr/core/src/java/org/apache/solr/search/SolrQueryTimeout.java, line > > 63 > > <https://reviews.apache.org/r/25658/diff/4/?file=691692#file691692line63> > > > > You may want to consider providing a little more detail in the comments > > that timeOutAt is the time in the future in nanos. Would it make sense to > > just pass the timeout offset here and have it calculate the future time > > within the set method? i.e. pass in 1000ms instead of current time + > > offset. Or another alternative is to provide a date/calendar object in the > > future (may be a bit overkill but then you don't need to think twice of if > > you need to pass in the time in millis or nanos). (Also applies to the > > QueryTimeout too) That makes sense, I'll improve that to do the math inside SolrQueryTimeout instead so you could just pass the timeAllowed value and it'd do the calculation. - Anshum ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25658/#review53648 ----------------------------------------------------------- On Sept. 17, 2014, 2:32 a.m., Anshum Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25658/ > ----------------------------------------------------------- > > (Updated Sept. 17, 2014, 2:32 a.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 > >
