LUCENE-3520: IndexReader.openIfChanged on NRT reader now returns null when there are no changes; some simplifications to SearcherManager

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1184877 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -622,6 +622,13 @@ Changes in backwards compatibility policy
   (instead of the old reader) if there are no changes in the index, to
   prevent the common pitfall of accidentally closing the old reader.
 
+Changes in runtime behavior
+
+* LUCENE-3520: IndexReader.openIfChanged, when passed a near-real-time
+  reader, will now return null if there are no changes.  The API has
+  always reserved the right to do this; it's just that in the past for
+  near-real-time readers it never did. (Mike McCandless)
+
 Bug fixes
 
 * LUCENE-3412: SloppyPhraseScorer was returning non-deterministic results 

lucene/contrib/misc/src/java/org/apache/lucene/index/NRTManager.java

@@ -93,10 +93,10 @@ public class NRTManager implements Closeable {
       SearcherWarmer warmer, boolean alwaysApplyDeletes) throws IOException {
     this.writer = writer;
     if (alwaysApplyDeletes) {
-      withoutDeletes = withDeletes = new SearcherManagerRef(true, 0,  SearcherManager.open(writer, true, warmer, es));
+      withoutDeletes = withDeletes = new SearcherManagerRef(true, 0,  new SearcherManager(writer, true, warmer, es));
     } else {
-      withDeletes = new SearcherManagerRef(true, 0,  SearcherManager.open(writer, true, warmer, es));
+      withDeletes = new SearcherManagerRef(true, 0, new SearcherManager(writer, true, warmer, es));
-      withoutDeletes = new SearcherManagerRef(false, 0,  SearcherManager.open(writer, false, warmer, es));
+      withoutDeletes = new SearcherManagerRef(false, 0, new SearcherManager(writer, false, warmer, es));
     }
     indexingGen = new AtomicLong(1);
   }

lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherLifetimeManager.java

@@ -173,8 +173,8 @@ public class SearcherLifetimeManager implements Closeable {
         // incRef done by SearcherTracker ctor:
         tracker.close();
       }
-    } else {
+    } else if (tracker.searcher != searcher) {
-      assert tracker.searcher == searcher;
+      throw new IllegalArgumentException("the provided searcher has the same underlying reader version yet the searcher instance differs from before (new=" + searcher + " vs old=" + tracker.searcher);
     }
 
     return version;

lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherManager.java

@@ -64,23 +64,76 @@ import org.apache.lucene.store.Directory;
  * @lucene.experimental
  */
 
-public abstract class SearcherManager {
+public final class SearcherManager {
 
-  protected volatile IndexSearcher currentSearcher;
+  private volatile IndexSearcher currentSearcher;
-  protected final ExecutorService es;
+  private final ExecutorService es;
-  protected final SearcherWarmer warmer;
+  private final SearcherWarmer warmer;
-  protected final Semaphore reopenLock = new Semaphore(1);
+  private final Semaphore reopenLock = new Semaphore(1);
   
-  protected SearcherManager(IndexReader openedReader, SearcherWarmer warmer,
+  /**
+   * Creates and returns a new SearcherManager from the given {@link IndexWriter}. 
+   * @param writer the IndexWriter to open the IndexReader from.
+   * @param applyAllDeletes If <code>true</code>, all buffered deletes will
+   *        be applied (made visible) in the {@link IndexSearcher} / {@link IndexReader}.
+   *        If <code>false</code>, the deletes may or may not be applied, but remain buffered 
+   *        (in IndexWriter) so that they will be applied in the future.
+   *        Applying deletes can be costly, so if your app can tolerate deleted documents
+   *        being returned you might gain some performance by passing <code>false</code>.
+   *        See {@link IndexReader#openIfChanged(IndexReader, IndexWriter, boolean)}.
+   * @param warmer An optional {@link SearcherWarmer}. Pass
+   *        <code>null</code> if you don't require the searcher to warmed
+   *        before going live.  If this is  <code>non-null</code> then a
+   *        merged segment warmer is installed on the
+   *        provided IndexWriter's config.
+   * @param es An optional {@link ExecutorService} so different segments can
+   *        be searched concurrently (see {@link
+   *        IndexSearcher#IndexSearcher(IndexReader,ExecutorService)}.  Pass <code>null</code>
+   *        to search segments sequentially.
+   *        
+   * @throws IOException
+   */
+  public SearcherManager(IndexWriter writer, boolean applyAllDeletes,
+      final SearcherWarmer warmer, final ExecutorService es) throws IOException {
+    this.es = es;
+    this.warmer = warmer;
+    currentSearcher = new IndexSearcher(IndexReader.open(writer, applyAllDeletes));
+    if (warmer != null) {
+      writer.getConfig().setMergedSegmentWarmer(
+          new IndexWriter.IndexReaderWarmer() {
+            @Override
+            public void warm(IndexReader reader) throws IOException {
+              warmer.warm(new IndexSearcher(reader, es));
+            }
+          });
+    }
+  }
+
+  /**
+   * Creates and returns a new SearcherManager from the given {@link Directory}. 
+   * @param dir the directory to open the IndexReader on.
+   * @param warmer An optional {@link SearcherWarmer}.  Pass
+   *        <code>null</code> if you don't require the searcher to warmed
+   *        before going live.  If this is  <code>non-null</code> then a
+   *        merged segment warmer is installed on the
+   *        provided IndexWriter's config.
+   * @param es And optional {@link ExecutorService} so different segments can
+   *        be searched concurrently (see {@link
+   *        IndexSearcher#IndexSearcher(IndexReader,ExecutorService)}.  Pass <code>null</code>
+   *        to search segments sequentially.
+   *        
+   * @throws IOException
+   */
+  public SearcherManager(Directory dir, SearcherWarmer warmer,
       ExecutorService es) throws IOException {
     this.es = es;
     this.warmer = warmer;
-    currentSearcher = new IndexSearcher(openedReader, es);
+    currentSearcher = new IndexSearcher(IndexReader.open(dir, true), es);
   }
 
   /**
    * You must call this, periodically, to perform a reopen. This calls
-   * {@link #openIfChanged(IndexReader)} with the underlying reader, and if that returns a
+   * {@link IndexReader#openIfChanged(IndexReader)} with the underlying reader, and if that returns a
    * new reader, it's warmed (if you provided a {@link SearcherWarmer} and then
    * swapped into production.
    * 
@@ -103,7 +156,9 @@ public abstract class SearcherManager {
     // threads just return immediately:
     if (reopenLock.tryAcquire()) {
       try {
-        final IndexReader newReader = openIfChanged(currentSearcher.getIndexReader());
+        // IR.openIfChanged preserves NRT and applyDeletes
+        // in the newly returned reader:
+        final IndexReader newReader = IndexReader.openIfChanged(currentSearcher.getIndexReader());
         if (newReader != null) {
           final IndexSearcher newSearcher = new IndexSearcher(newReader, es);
           boolean success = false;
@@ -190,122 +245,10 @@ public abstract class SearcherManager {
     }
   }
 
-  protected synchronized void swapSearcher(IndexSearcher newSearcher) throws IOException {
+  private synchronized void swapSearcher(IndexSearcher newSearcher) throws IOException {
     ensureOpen();
     final IndexSearcher oldSearcher = currentSearcher;
     currentSearcher = newSearcher;
     release(oldSearcher);
   }
-
-  protected abstract IndexReader openIfChanged(IndexReader oldReader)
-      throws IOException;
-
-  /**
-   * Creates and returns a new SearcherManager from the given {@link IndexWriter}. 
-   * @param writer the IndexWriter to open the IndexReader from.
-   * @param applyAllDeletes If <code>true</code>, all buffered deletes will
-   *        be applied (made visible) in the {@link IndexSearcher} / {@link IndexReader}.
-   *        If <code>false</code>, the deletes are not applied but remain buffered 
-   *        (in IndexWriter) so that they will be applied in the future.
-   *        Applying deletes can be costly, so if your app can tolerate deleted documents
-   *        being returned you might gain some performance by passing <code>false</code>.
-   * @param warmer An optional {@link SearcherWarmer}. Pass
-   *        <code>null</code> if you don't require the searcher to warmed
-   *        before going live.  If this is  <code>non-null</code> then a
-   *        merged segment warmer is installed on the
-   *        provided IndexWriter's config.
-   * @param es An optional {@link ExecutorService} so different segments can
-   *        be searched concurrently (see {@link
-   *        IndexSearcher#IndexSearcher(IndexReader,ExecutorService)}.  Pass <code>null</code>
-   *        to search segments sequentially.
-   *        
-   * @see IndexReader#openIfChanged(IndexReader, IndexWriter, boolean)
-   * @throws IOException
-   */
-  public static SearcherManager open(IndexWriter writer, boolean applyAllDeletes,
-      SearcherWarmer warmer, ExecutorService es) throws IOException {
-    final IndexReader open = IndexReader.open(writer, true);
-    boolean success = false;
-    try {
-      SearcherManager manager = new NRTSearcherManager(writer, applyAllDeletes,
-          open, warmer, es);
-      success = true;
-      return manager;
-    } finally {
-      if (!success) {
-        open.close();
-      }
-    }
-  }
-
-  /**
-   * Creates and returns a new SearcherManager from the given {@link Directory}. 
-   * @param dir the directory to open the IndexReader on.
-   * @param warmer An optional {@link SearcherWarmer}.  Pass
-   *        <code>null</code> if you don't require the searcher to warmed
-   *        before going live.  If this is  <code>non-null</code> then a
-   *        merged segment warmer is installed on the
-   *        provided IndexWriter's config.
-   * @param es And optional {@link ExecutorService} so different segments can
-   *        be searched concurrently (see {@link
-   *        IndexSearcher#IndexSearcher(IndexReader,ExecutorService)}.  Pass <code>null</code>
-   *        to search segments sequentially.
-   *        
-   * @throws IOException
-   */
-  public static SearcherManager open(Directory dir, SearcherWarmer warmer,
-      ExecutorService es) throws IOException {
-    final IndexReader open = IndexReader.open(dir, true);
-    boolean success = false;
-    try {
-      SearcherManager manager = new DirectorySearchManager(open, warmer, es);
-      success = true;
-      return manager;
-    } finally {
-      if (!success) {
-        open.close();
-      }
-    }
-  }
-
-  static final class NRTSearcherManager extends SearcherManager {
-    private final IndexWriter writer;
-    private final boolean applyDeletes;
-
-    NRTSearcherManager(final IndexWriter writer, final boolean applyDeletes,
-        final IndexReader openedReader, final SearcherWarmer warmer, final ExecutorService es)
-        throws IOException {
-      super(openedReader, warmer, es);
-      this.writer = writer;
-      this.applyDeletes = applyDeletes;
-      if (warmer != null) {
-        writer.getConfig().setMergedSegmentWarmer(
-            new IndexWriter.IndexReaderWarmer() {
-              @Override
-              public void warm(IndexReader reader) throws IOException {
-                warmer.warm(new IndexSearcher(reader, es));
-              }
-            });
-      }
-    }
-
-    @Override
-    protected IndexReader openIfChanged(IndexReader oldReader)
-        throws IOException {
-      return IndexReader.openIfChanged(oldReader, writer, applyDeletes);
-    }
-  }
-
-  static final class DirectorySearchManager extends SearcherManager {
-    DirectorySearchManager(IndexReader openedReader,
-        SearcherWarmer warmer, ExecutorService es) throws IOException {
-      super(openedReader, warmer, es);
-    }
-
-    @Override
-    protected IndexReader openIfChanged(IndexReader oldReader)
-        throws IOException {
-      return IndexReader.openIfChanged(oldReader, true);
-    }
-  }
 }

lucene/contrib/misc/src/test/org/apache/lucene/search/TestSearcherManager.java

@@ -72,12 +72,12 @@ public class TestSearcherManager extends ThreadedIndexingAndSearchingTestCase {
       }
     };
     if (random.nextBoolean()) {
-      mgr = SearcherManager.open(writer, true, warmer, es);
+      mgr = new SearcherManager(writer, true, warmer, es);
       isNRT = true;
     } else {
       // SearcherManager needs to see empty commit:
       writer.commit();
-      mgr = SearcherManager.open(dir, warmer, es);
+      mgr = new SearcherManager(dir, warmer, es);
       isNRT = false;
     }
     
@@ -198,8 +198,8 @@ public class TestSearcherManager extends ThreadedIndexingAndSearchingTestCase {
         }
       }
     };
-    final SearcherManager searcherManager = random.nextBoolean() ? SearcherManager.open(dir,
+    final SearcherManager searcherManager = random.nextBoolean() ? new SearcherManager(dir,
-        warmer, es) : SearcherManager.open(writer, random.nextBoolean(), warmer, es);
+        warmer, es) : new SearcherManager(writer, random.nextBoolean(), warmer, es);
     IndexSearcher searcher = searcherManager.acquire();
     try {
       assertEquals(1, searcher.getIndexReader().numDocs());

lucene/src/java/org/apache/lucene/index/DirectoryReader.java

@@ -406,8 +406,15 @@ class DirectoryReader extends IndexReader implements Cloneable {
     return doOpenIfChanged(true, commit);
   }
 
-  // NOTE: always returns a non-null result (ie new reader)
+  @Override
-  // but that could change someday
+  protected final IndexReader doOpenIfChanged(IndexWriter writer, boolean applyAllDeletes) throws CorruptIndexException, IOException {
+    if (writer == this.writer && applyAllDeletes == this.applyAllDeletes) {
+      return doOpenIfChanged();
+    } else {    
+      return super.doOpenIfChanged(writer, applyAllDeletes);
+    }
+  }
+
   private final IndexReader doOpenFromWriter(boolean openReadOnly, IndexCommit commit) throws CorruptIndexException, IOException {
     assert readOnly;
 
@@ -419,10 +426,18 @@ class DirectoryReader extends IndexReader implements Cloneable {
       throw new IllegalArgumentException("a reader obtained from IndexWriter.getReader() cannot currently accept a commit");
     }
 
-    // TODO: right now we *always* make a new reader; in
+    if (writer.nrtIsCurrent(segmentInfos)) {
-    // the future we could have write make some effort to
+      return null;
-    // detect that no changes have occurred
+    }
+
     IndexReader reader = writer.getReader(applyAllDeletes);
+
+    // If in fact no changes took place, return null:
+    if (reader.getVersion() == getVersion()) {
+      reader.decRef();
+      return null;
+    }
+
     reader.readerFinishedListeners = readerFinishedListeners;
     return reader;
   }

lucene/src/java/org/apache/lucene/index/IndexReader.java

@@ -561,10 +561,6 @@ public abstract class IndexReader implements Cloneable,Closeable {
    * with the old reader uses "copy on write" semantics to
    * ensure the changes are not seen by other readers.
    *
-   * <p><b>NOTE</b>: If the provided reader is a near real-time
-   * reader, this method will return another near-real-time
-   * reader.
-   * 
    * @throws CorruptIndexException if the index is corrupt
    * @throws IOException if there is a low-level IO error
    * @return null if there are no changes; else, a new

lucene/src/java/org/apache/lucene/index/IndexWriter.java

@@ -4073,6 +4073,7 @@ public class IndexWriter implements Closeable, TwoPhaseCommit {
 
   synchronized boolean nrtIsCurrent(SegmentInfos infos) {
     //System.out.println("IW.nrtIsCurrent " + (infos.version == segmentInfos.version && !docWriter.anyChanges() && !bufferedDeletesStream.any()));
+    ensureOpen();
     return infos.version == segmentInfos.version && !docWriter.anyChanges() && !bufferedDeletesStream.any();
   }
 

lucene/src/test-framework/org/apache/lucene/index/ThreadedIndexingAndSearchingTestCase.java

@@ -55,7 +55,7 @@ import org.apache.lucene.util._TestUtil;
 
 // TODO
 //   - mix in optimize, addIndexes
-//   - randomoly mix in non-congruent docs
+//   - randomly mix in non-congruent docs
 
 /** Utility class that spawns multiple indexing and
  *  searching threads. */

lucene/src/test/org/apache/lucene/index/TestIndexWriterReader.java

@@ -857,7 +857,7 @@ public class TestIndexWriterReader extends LuceneTestCase {
     int sum = 0;
     while(System.currentTimeMillis() < endTime) {
       IndexReader r2 = IndexReader.openIfChanged(r);
-      if (r2 != r) {
+      if (r2 != null) {
         r.close();
         r = r2;
       }
@@ -1016,4 +1016,40 @@ public class TestIndexWriterReader extends LuceneTestCase {
     }
   }
   
+  public void testReopenAfterNoRealChange() throws Exception {
+    Directory d = newDirectory();
+    IndexWriter w = new IndexWriter(
+        d,
+        newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random)));
+    w.setInfoStream(VERBOSE ? System.out : null);
+
+    IndexReader r = w.getReader(); // start pooling readers
+
+    IndexReader r2 = IndexReader.openIfChanged(r);
+    assertNull(r2);
+    
+    w.addDocument(new Document());
+    IndexReader r3 = IndexReader.openIfChanged(r);
+    assertNotNull(r3);
+    assertTrue(r3.getVersion() != r.getVersion());
+    assertTrue(r3.isCurrent());
+
+    // Deletes nothing in reality...:
+    w.deleteDocuments(new Term("foo", "bar"));
+
+    // ... but IW marks this as not current:
+    assertFalse(r3.isCurrent());
+    IndexReader r4 = IndexReader.openIfChanged(r3);
+    assertNull(r4);
+
+    // Deletes nothing in reality...:
+    w.deleteDocuments(new Term("foo", "bar"));
+    IndexReader r5 = IndexReader.openIfChanged(r3, w, true);
+    assertNull(r5);
+
+    r3.close();
+
+    w.close();
+    d.close();
+  }
 }

lucene/src/test/org/apache/lucene/search/TestCachingSpanFilter.java

@@ -116,10 +116,10 @@ public class TestCachingSpanFilter extends LuceneTestCase {
 
     // make sure we get a cache hit when we reopen readers
     // that had no new deletions
+    // Deletes nothing:
+    writer.deleteDocuments(new Term("foo", "bar"));
     reader = refreshReader(reader);
-    assertTrue(reader != oldReader);
+    assertTrue(reader == oldReader);
-    searcher.close();
-    searcher = newSearcher(reader, false);
     int missCount = filter.missCount;
     docs = searcher.search(constantScore, 1);
     assertEquals("[just filter] Should find a hit...", 1, docs.totalHits);

lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java

@@ -235,10 +235,9 @@ public class TestCachingWrapperFilter extends LuceneTestCase {
 
     // make sure we get a cache hit when we reopen reader
     // that had no change to deletions
+    writer.deleteDocuments(new Term("foo", "bar"));
     reader = refreshReader(reader);
-    assertTrue(reader != oldReader);
+    assertTrue(reader == oldReader);
-    searcher.close();
-    searcher = newSearcher(reader, false);
     int missCount = filter.missCount;
     docs = searcher.search(constantScore, 1);
     assertEquals("[just filter] Should find a hit...", 1, docs.totalHits);