You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Anshum Gupta <an...@apache.org> on 2014/09/15 22:04:52 UTC
Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/
-----------------------------------------------------------
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/test/org/apache/lucene/index/TestExitableDirectoryReader.java PRE-CREATION
trunk/solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java 1625118
trunk/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java 1625118
trunk/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java 1625118
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
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Steve Davids <sd...@gmail.com>.
> 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.
>
> Anshum Gupta wrote:
> shouldExit() is more intuitive for users to implement when they want their own custom logic for exiting the queries.
Wouldn't it make sense to just go the interface route then? Not sure what the abstract class is doing for you at this point.
> 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?
>
> Anshum Gupta wrote:
> 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.
Brain fart - yes threads can be pooled within the container and may service multiple requests over the thread life cycle.
- Steve
-----------------------------------------------------------
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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
> 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.
>
> Anshum Gupta wrote:
> shouldExit() is more intuitive for users to implement when they want their own custom logic for exiting the queries.
>
> Steve Davids wrote:
> Wouldn't it make sense to just go the interface route then? Not sure what the abstract class is doing for you at this point.
Done :).
> 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?
>
> Anshum Gupta wrote:
> 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.
>
> Steve Davids wrote:
> Brain fart - yes threads can be pooled within the container and may service multiple requests over the thread life cycle.
Even if you wouldn't want to use a pool, ThreadLocal would leak the values for threads that are already done processing the request. So..
- Anshum
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/#review53648
-----------------------------------------------------------
On Sept. 17, 2014, 5:25 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, 5:25 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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
> 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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
> 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)
>
> Anshum Gupta wrote:
> 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.
This is now done and a part of the latest patch.
- Anshum
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/#review53648
-----------------------------------------------------------
On Sept. 17, 2014, 5:25 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, 5:25 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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Steve Davids <sd...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/#review53648
-----------------------------------------------------------
trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeout.java
<https://reviews.apache.org/r/25658/#comment93367>
You can provide the shouldExit implementation in the abstract class if you make the getTimeoutAt() abstract.
trunk/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
<https://reviews.apache.org/r/25658/#comment93366>
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?
trunk/solr/core/src/java/org/apache/solr/search/SolrQueryTimeout.java
<https://reviews.apache.org/r/25658/#comment93365>
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)
- Steve Davids
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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/
-----------------------------------------------------------
(Updated Sept. 25, 2014, 7:16 p.m.)
Review request for lucene.
Changes
-------
Patch that integrates Steve's changes and also changes for AtomicReader -> LeafReader refactor.
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 (updated)
-----
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/QueryTimeoutImpl.java PRE-CREATION
trunk/lucene/core/src/java/org/apache/lucene/search/TimeLimitingCollector.java 1627584
trunk/lucene/core/src/test/org/apache/lucene/index/TestExitableDirectoryReader.java PRE-CREATION
trunk/lucene/core/src/test/org/apache/lucene/search/TestTimeLimitingCollector.java 1627584
trunk/solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java 1627584
trunk/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java 1627584
trunk/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java 1627584
trunk/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java 1627584
trunk/solr/core/src/java/org/apache/solr/search/SolrQueryTimeoutImpl.java PRE-CREATION
trunk/solr/core/src/test/org/apache/solr/TestDistributedSearch.java 1627584
trunk/solr/core/src/test/org/apache/solr/TestGroupingSearch.java 1627584
trunk/solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java PRE-CREATION
trunk/solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java PRE-CREATION
trunk/solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java 1627584
Diff: https://reviews.apache.org/r/25658/diff/
Testing
-------
Added Lucene/Solr tests. Tested a bit manually.
Thanks,
Anshum Gupta
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 5:25 a.m.)
Review request for lucene.
Changes
-------
The QueryTimeoutBase is now an interface and SolrQueryTimeout and QueryTimeout implement the mandatory shouldExit().
Also, the constructor and setter for the implementations now accept timeAllowed in ms (as with TimeLimitingCollector) and set the internal timeoutAt value themselves.
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 (updated)
-----
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
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Addressing the feedback from the last review.
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 (updated)
-----
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
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
> 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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Robert Muir <rc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/#review53638
-----------------------------------------------------------
trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
<https://reviews.apache.org/r/25658/#comment93345>
Can we somehow share this with TimeLimitingCollector.TimeExceededException?
trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
<https://reviews.apache.org/r/25658/#comment93347>
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 :)
trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
<https://reviews.apache.org/r/25658/#comment93348>
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.
trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeout.java
<https://reviews.apache.org/r/25658/#comment93351>
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.
trunk/lucene/core/src/test/org/apache/lucene/index/TestExitableDirectoryReader.java
<https://reviews.apache.org/r/25658/#comment93350>
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?
- Robert Muir
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
>
>
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Changed the design to have an abstract/base QueryTimeout class and implementations that use local var and ThreadLocal for Lucene/Solr. Changed the Lucene test to use the non-ThreadLocal implementation and SolrIndexSearcher uses the ThreadLocal implementation of QueryTimeout.
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 (updated)
-----
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
Re: Review Request 25658: Timeout queries when they take too long to
rewrite/enumerate over terms.
Posted by Anshum Gupta <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25658/
-----------------------------------------------------------
(Updated Sept. 15, 2014, 9:02 p.m.)
Review request for lucene.
Changes
-------
Updated the patch after integrating most of Roberts' recommended changes.
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 (updated)
-----
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/test/org/apache/lucene/index/TestExitableDirectoryReader.java PRE-CREATION
trunk/solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java 1625118
trunk/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java 1625118
trunk/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java 1625118
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