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

trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
40
  public static Logger log = Logger.getLogger("ExitableDirectoryReader");

What is the purpose of this? Lucene is an API, it doesnt do logging. it also seems unusued.


trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
81
    public AtomicReader getOriginalReader() {

Someone can just use FilterAtomicReader.unwrap() ?


trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
93

All of these methods that return in.XXX are unnecessary. FilterAtomicReader does that already.


trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
139
  public static class ExitableFields extends Fields {

This should extend FilterFields. The _ notation is strange.


trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
172
  public static class ExitableTerms extends Terms {

This should extend FilterTerms


trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
248
    public ExitableTermsEnum(TermsEnum termsEnum) {

This should extend FilterTermsEnum


trunk/lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java (Diff revision 1)
254
      if (QueryTimeout.shouldExit()) {

I don't agree with the 'global static threadlocal QueryTimeOut' class, why use this approach? This thing can just take an instance instead.


trunk/lucene/core/src/java/org/apache/lucene/index/QueryTimeout.java (Diff revision 1)
31
  public static ThreadLocal<Long> _timeOutAt = new ThreadLocal() {

There is no way to close this threadlocal (except gc), and i dont think the timeout processing should be done with threadlocal anyway. Instead why cant the reader just take a timeout object? FilterReaders are cheap, you could even make one for each query to keep things simple and contained.


trunk/lucene/core/src/test/org/apache/lucene/index/TestExitableDirectoryReader.java (Diff revision 1)
125
    // Set a really low timeOut value (1 ms) and expect an Exception

It looks like this test depends on wall-clock time? Can it be mocked instead? I dont want the false failures because the test "ran too fast"


- Robert Muir


On September 15th, 2014, 7:45 p.m. UTC, Anshum Gupta wrote:

Review request for lucene.
By Anshum Gupta.

Updated Sept. 15, 2014, 7:45 p.m.

Bugs: SOLR-5986
Repository: lucene

Description

Latest patch

Testing

Manually tested and tests for lucene as well as Solr.

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)

View Diff