Index: lucene/contrib/misc/src/test/org/apache/lucene/search/TestSearcherManager.java =================================================================== --- lucene/contrib/misc/src/test/org/apache/lucene/search/TestSearcherManager.java (revision 1183753) +++ lucene/contrib/misc/src/test/org/apache/lucene/search/TestSearcherManager.java (working copy) @@ -72,12 +72,12 @@ } }; 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 @@ } } }; - final SearcherManager searcherManager = random.nextBoolean() ? SearcherManager.open(dir, - warmer, es) : SearcherManager.open(writer, random.nextBoolean(), warmer, es); + final SearcherManager searcherManager = random.nextBoolean() ? new SearcherManager(dir, + warmer, es) : new SearcherManager(writer, random.nextBoolean(), warmer, es); IndexSearcher searcher = searcherManager.acquire(); try { assertEquals(1, searcher.getIndexReader().numDocs()); Index: lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherLifetimeManager.java =================================================================== --- lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherLifetimeManager.java (revision 1183650) +++ lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherLifetimeManager.java (working copy) @@ -173,8 +173,8 @@ // incRef done by SearcherTracker ctor: tracker.close(); } - } else { - assert tracker.searcher == searcher; + } else if (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; Index: lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherManager.java =================================================================== --- lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherManager.java (revision 1183650) +++ lucene/contrib/misc/src/java/org/apache/lucene/search/SearcherManager.java (working copy) @@ -64,23 +64,79 @@ * @lucene.experimental */ -public abstract class SearcherManager { +public final class SearcherManager { - protected volatile IndexSearcher currentSearcher; - protected final ExecutorService es; - protected final SearcherWarmer warmer; - protected final Semaphore reopenLock = new Semaphore(1); + private volatile IndexSearcher currentSearcher; + private final ExecutorService es; + private final SearcherWarmer warmer; + private final Semaphore reopenLock = new Semaphore(1); + private final IndexWriter writer; - 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 true, all buffered deletes will + * be applied (made visible) in the {@link IndexSearcher} / {@link IndexReader}. + * If false, 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 false. + * See {@link IndexReader#openIfChanged(IndexReader, IndexWriter, boolean)}. + * @param warmer An optional {@link SearcherWarmer}. Pass + * null if you don't require the searcher to warmed + * before going live. If this is non-null 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 null + * 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; + this.writer = writer; + 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 + * null if you don't require the searcher to warmed + * before going live. If this is non-null 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 null + * 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); + writer = null; } /** * 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 +159,9 @@ // 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 +248,10 @@ } } - 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 true, all buffered deletes will - * be applied (made visible) in the {@link IndexSearcher} / {@link IndexReader}. - * If false, 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 false. - * @param warmer An optional {@link SearcherWarmer}. Pass - * null if you don't require the searcher to warmed - * before going live. If this is non-null 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 null - * 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 - * null if you don't require the searcher to warmed - * before going live. If this is non-null 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 null - * 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); - } - } } Index: lucene/contrib/misc/src/java/org/apache/lucene/index/NRTManager.java =================================================================== --- lucene/contrib/misc/src/java/org/apache/lucene/index/NRTManager.java (revision 1183650) +++ lucene/contrib/misc/src/java/org/apache/lucene/index/NRTManager.java (working copy) @@ -93,10 +93,10 @@ 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)); - withoutDeletes = new SearcherManagerRef(false, 0, SearcherManager.open(writer, false, warmer, es)); + withDeletes = new SearcherManagerRef(true, 0, new SearcherManager(writer, true, warmer, es)); + withoutDeletes = new SearcherManagerRef(false, 0, new SearcherManager(writer, false, warmer, es)); } indexingGen = new AtomicLong(1); } Index: lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java =================================================================== --- lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java (revision 1183650) +++ lucene/src/test/org/apache/lucene/search/TestCachingWrapperFilter.java (working copy) @@ -235,10 +235,9 @@ // 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); - searcher.close(); - searcher = newSearcher(reader, false); + assertTrue(reader == oldReader); int missCount = filter.missCount; docs = searcher.search(constantScore, 1); assertEquals("[just filter] Should find a hit...", 1, docs.totalHits); Index: lucene/src/test/org/apache/lucene/search/TestCachingSpanFilter.java =================================================================== --- lucene/src/test/org/apache/lucene/search/TestCachingSpanFilter.java (revision 1183650) +++ lucene/src/test/org/apache/lucene/search/TestCachingSpanFilter.java (working copy) @@ -116,10 +116,10 @@ // 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); - searcher.close(); - searcher = newSearcher(reader, false); + assertTrue(reader == oldReader); int missCount = filter.missCount; docs = searcher.search(constantScore, 1); assertEquals("[just filter] Should find a hit...", 1, docs.totalHits); Index: lucene/src/test/org/apache/lucene/index/TestIndexWriterReader.java =================================================================== --- lucene/src/test/org/apache/lucene/index/TestIndexWriterReader.java (revision 1183650) +++ lucene/src/test/org/apache/lucene/index/TestIndexWriterReader.java (working copy) @@ -857,7 +857,7 @@ 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 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(); + } } Index: lucene/src/java/org/apache/lucene/index/DirectoryReader.java =================================================================== --- lucene/src/java/org/apache/lucene/index/DirectoryReader.java (revision 1183650) +++ lucene/src/java/org/apache/lucene/index/DirectoryReader.java (working copy) @@ -406,8 +406,15 @@ return doOpenIfChanged(true, commit); } - // NOTE: always returns a non-null result (ie new reader) - // but that could change someday + @Override + 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 @@ throw new IllegalArgumentException("a reader obtained from IndexWriter.getReader() cannot currently accept a commit"); } - // TODO: right now we *always* make a new reader; in - // the future we could have write make some effort to - // detect that no changes have occurred + if (writer.nrtIsCurrent(segmentInfos)) { + return null; + } + 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; } Index: lucene/src/java/org/apache/lucene/index/IndexReader.java =================================================================== --- lucene/src/java/org/apache/lucene/index/IndexReader.java (revision 1183650) +++ lucene/src/java/org/apache/lucene/index/IndexReader.java (working copy) @@ -561,10 +561,6 @@ * with the old reader uses "copy on write" semantics to * ensure the changes are not seen by other readers. * - *

NOTE: 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 Index: lucene/src/java/org/apache/lucene/index/IndexWriter.java =================================================================== --- lucene/src/java/org/apache/lucene/index/IndexWriter.java (revision 1183650) +++ lucene/src/java/org/apache/lucene/index/IndexWriter.java (working copy) @@ -4073,6 +4073,7 @@ 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(); } Index: lucene/src/test-framework/org/apache/lucene/index/ThreadedIndexingAndSearchingTestCase.java =================================================================== --- lucene/src/test-framework/org/apache/lucene/index/ThreadedIndexingAndSearchingTestCase.java (revision 1183650) +++ lucene/src/test-framework/org/apache/lucene/index/ThreadedIndexingAndSearchingTestCase.java (working copy) @@ -55,7 +55,7 @@ // 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. */