Index: lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java =================================================================== --- lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java (revision 1241912) +++ lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java (working copy) @@ -55,7 +55,7 @@ if (!isNRT) { writer.commit(); } - assertTrue(mgr.maybeReopen() || mgr.isSearcherCurrent()); + assertTrue(mgr.maybeRefresh() || mgr.isSearcherCurrent()); return mgr.acquire(); } @@ -101,7 +101,7 @@ Thread.sleep(_TestUtil.nextInt(random, 1, 100)); writer.commit(); Thread.sleep(_TestUtil.nextInt(random, 1, 5)); - if (mgr.maybeReopen()) { + if (mgr.maybeRefresh()) { lifetimeMGR.prune(pruner); } } @@ -130,7 +130,7 @@ // synchronous to your search threads, but still we // test as apps will presumably do this for // simplicity: - if (mgr.maybeReopen()) { + if (mgr.maybeRefresh()) { lifetimeMGR.prune(pruner); } } @@ -196,6 +196,7 @@ final AtomicBoolean triedReopen = new AtomicBoolean(false); final ExecutorService es = random.nextBoolean() ? null : Executors.newCachedThreadPool(new NamedThreadFactory("testIntermediateClose")); final SearcherFactory factory = new SearcherFactory() { + @Override public IndexSearcher newSearcher(IndexReader r) throws IOException { try { if (triedReopen.get()) { @@ -232,7 +233,7 @@ if (VERBOSE) { System.out.println("NOW call maybeReopen"); } - searcherManager.maybeReopen(); + searcherManager.maybeRefresh(); success.set(true); } catch (AlreadyClosedException e) { // expected Index: lucene/core/src/java/org/apache/lucene/search/ThingyManager.java =================================================================== --- lucene/core/src/java/org/apache/lucene/search/ThingyManager.java (revision 0) +++ lucene/core/src/java/org/apache/lucene/search/ThingyManager.java (working copy) @@ -0,0 +1,149 @@ +package org.apache.lucene.search; + +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import java.io.Closeable; +import java.io.IOException; +import java.util.concurrent.Semaphore; + +import org.apache.lucene.store.AlreadyClosedException; + +public abstract class ThingyManager implements Closeable { + + private static final String THINGY_MANAGER_IS_CLOSED = "this ThingyManager is closed"; + + private final Semaphore reopenLock = new Semaphore(1); + protected G current; + + private void ensureOpen() { + if (current == null) { + throw new AlreadyClosedException(THINGY_MANAGER_IS_CLOSED); + } + } + + private synchronized void swapThingy(G newThingy) throws IOException { + ensureOpen(); + final G oldThingy = current; + current = newThingy; + release(oldThingy); + } + + /** Decrement reference counting on the given reference. */ + protected abstract void decRef(G thingy) throws IOException; + + /** + * Refresh the given reference if needed. Returns {@code null} if not refresh + * was needed, otherwise a new refreshed reference. + */ + protected abstract G refreshIfNeeded(G thingyToRefresh) throws IOException; + + /** + * Try to increment reference counting on the given reference. Return true if + * the operation was successful. + */ + protected abstract boolean tryIncRef(G thingy); + + /** + * Obtain the current thingy. You must match every call to acquire with one + * call to {@link #release}; it's best to do so in a finally clause, and set + * the reference to {@code null} to prevent accidental usage after it has been + * released. + */ + public final G acquire() { + G thingy; + do { + if ((thingy = current) == null) { + throw new AlreadyClosedException(THINGY_MANAGER_IS_CLOSED); + } + } while (!tryIncRef(thingy)); + return thingy; + } + + /** + * Close this ThingyManager to future {@link #acquire() acquiring}. Any + * references that were previously {@link #acquire() acquired} won't be + * affected, and they should still be {@link #release released} when they are + * not needed anymore. + */ + public final synchronized void close() throws IOException { + if (current != null) { + // make sure we can call this more than once + // closeable javadoc says: + // if this is already closed then invoking this method has no effect. + swapThingy(null); + } + } + + /** + * You must call this, periodically, if you want that {@link #acquire()} will + * return refreshed instances. + * + *

+ * Threads: it's fine for more than one thread to call this at once. + * Only the first thread will attempt the refresh; subsequent threads will see + * that another thread is already handling refresh and will return + * immediately. Note that this means if another thread is already refreshing + * then subsequent threads will return right away without waiting for the + * refresh to complete. + * + *

+ * This method returns true if the reference was in fact refreshed, or if the + * current reference has no pending changes. + */ + public final boolean maybeRefresh() throws IOException { + ensureOpen(); + // Ensure only 1 thread does reopen at once; other threads just return immediately: + if (reopenLock.tryAcquire()) { + try { + final G thingy = acquire(); + try { + G newThingy = refreshIfNeeded(thingy); + if (newThingy != null) { + boolean success = false; + try { + swapThingy(newThingy); + success = true; + } finally { + if (!success) { + release(newThingy); + } + } + } + } finally { + release(thingy); + } + return true; + } finally { + reopenLock.release(); + } + } else { + return false; + } + } + + /** + * Release the refernce previously obtained via {@link #acquire()}. + *

+ * NOTE: it's safe to call this after {@link #close()}. + */ + public final void release(G thingy) throws IOException { + assert thingy != null; + decRef(thingy); + } + +} Property changes on: lucene/core/src/java/org/apache/lucene/search/ThingyManager.java ___________________________________________________________________ Added: svn:executable ## -0,0 +1 ## +* Added: svn:eol-style ## -0,0 +1 ## +native Index: lucene/core/src/java/org/apache/lucene/search/SearcherManager.java =================================================================== --- lucene/core/src/java/org/apache/lucene/search/SearcherManager.java (revision 1241965) +++ lucene/core/src/java/org/apache/lucene/search/SearcherManager.java (working copy) @@ -17,16 +17,10 @@ * limitations under the License. */ -import java.io.Closeable; import java.io.IOException; -import java.util.concurrent.Semaphore; -import org.apache.lucene.index.CorruptIndexException; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.IndexWriter; -import org.apache.lucene.search.NRTManager; // javadocs -import org.apache.lucene.search.IndexSearcher; // javadocs -import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.Directory; /** @@ -50,7 +44,7 @@ * * *

- * In addition you should periodically call {@link #maybeReopen}. While it's + * In addition you should periodically call {@link #maybeRefresh}. While it's * possible to call this just before running each query, this is discouraged * since it penalizes the unlucky queries that do the reopen. It's better to use * a separate background thread, that periodically calls maybeReopen. Finally, @@ -65,12 +59,9 @@ * * @lucene.experimental */ +public final class SearcherManager extends ThingyManager { -public final class SearcherManager implements Closeable { - - private volatile IndexSearcher currentSearcher; private final SearcherFactory searcherFactory; - private final Semaphore reopenLock = new Semaphore(1); /** * Creates and returns a new SearcherManager from the given {@link IndexWriter}. @@ -93,8 +84,28 @@ searcherFactory = new SearcherFactory(); } this.searcherFactory = searcherFactory; - currentSearcher = searcherFactory.newSearcher(IndexReader.open(writer, applyAllDeletes)); + current = searcherFactory.newSearcher(IndexReader.open(writer, applyAllDeletes)); } + + @Override + protected void decRef(IndexSearcher thingy) throws IOException { + thingy.getIndexReader().decRef(); + } + + @Override + protected IndexSearcher refreshIfNeeded(IndexSearcher thingyToRefresh) throws IOException { + final IndexReader newReader = IndexReader.openIfChanged(thingyToRefresh.getIndexReader()); + if (newReader == null) { + return null; + } else { + return searcherFactory.newSearcher(newReader); + } + } + + @Override + protected boolean tryIncRef(IndexSearcher thingy) { + return thingy.getIndexReader().tryIncRef(); + } /** * Creates and returns a new SearcherManager from the given {@link Directory}. @@ -110,72 +121,71 @@ searcherFactory = new SearcherFactory(); } this.searcherFactory = searcherFactory; - currentSearcher = searcherFactory.newSearcher(IndexReader.open(dir)); + current = searcherFactory.newSearcher(IndexReader.open(dir)); } - /** - * You must call this, periodically, to perform a reopen. This calls - * {@link IndexReader#openIfChanged(IndexReader)} with the underlying reader, and if that returns a - * new reader, it's warmed (if you provided a {@link SearcherFactory} and then - * swapped into production. - * - *

- * Threads: it's fine for more than one thread to call this at once. - * Only the first thread will attempt the reopen; subsequent threads will see - * that another thread is already handling reopen and will return immediately. - * Note that this means if another thread is already reopening then subsequent - * threads will return right away without waiting for the reader reopen to - * complete. - *

- * - *

- * This method returns true if a new reader was in fact opened or - * if the current searcher has no pending changes. - *

- */ - public boolean maybeReopen() throws IOException { - ensureOpen(); - // Ensure only 1 thread does reopen at once; other - // threads just return immediately: - if (reopenLock.tryAcquire()) { - try { - // IR.openIfChanged preserves NRT and applyDeletes - // in the newly returned reader: - final IndexReader newReader; - final IndexSearcher searcherToReopen = acquire(); - try { - newReader = IndexReader.openIfChanged(searcherToReopen.getIndexReader()); - } finally { - release(searcherToReopen); - } - if (newReader != null) { - final IndexSearcher newSearcher = searcherFactory.newSearcher(newReader); - boolean success = false; - try { - swapSearcher(newSearcher); - success = true; - } finally { - if (!success) { - release(newSearcher); - } - } - } - return true; - } finally { - reopenLock.release(); - } - } else { - return false; - } - } +// /** +// * You must call this, periodically, to perform a reopen. This calls +// * {@link IndexReader#openIfChanged(IndexReader)} with the underlying reader, and if that returns a +// * new reader, it's warmed (if you provided a {@link SearcherFactory} and then +// * swapped into production. +// * +// *

+// * Threads: it's fine for more than one thread to call this at once. +// * Only the first thread will attempt the reopen; subsequent threads will see +// * that another thread is already handling reopen and will return immediately. +// * Note that this means if another thread is already reopening then subsequent +// * threads will return right away without waiting for the reader reopen to +// * complete. +// *

+// * +// *

+// * This method returns true if a new reader was in fact opened or +// * if the current searcher has no pending changes. +// *

+// */ +// public boolean maybeReopen() throws IOException { +// ensureOpen(); +// // Ensure only 1 thread does reopen at once; other +// // threads just return immediately: +// if (reopenLock.tryAcquire()) { +// try { +// // IR.openIfChanged preserves NRT and applyDeletes +// // in the newly returned reader: +// final IndexReader newReader; +// final IndexSearcher searcherToReopen = acquire(); +// try { +// newReader = IndexReader.openIfChanged(searcherToReopen.getIndexReader()); +// } finally { +// release(searcherToReopen); +// } +// if (newReader != null) { +// final IndexSearcher newSearcher = searcherFactory.newSearcher(newReader); +// boolean success = false; +// try { +// swapSearcher(newSearcher); +// success = true; +// } finally { +// if (!success) { +// release(newSearcher); +// } +// } +// } +// return true; +// } finally { +// reopenLock.release(); +// } +// } else { +// return false; +// } +// } /** * Returns true if no changes have occured since this searcher * ie. reader was opened, otherwise false. * @see IndexReader#isCurrent() */ - public boolean isSearcherCurrent() throws CorruptIndexException, - IOException { + public boolean isSearcherCurrent() throws IOException { final IndexSearcher searcher = acquire(); try { return searcher.getIndexReader().isCurrent(); @@ -183,56 +193,57 @@ release(searcher); } } +// +// /** +// * Release the searcher previously obtained with {@link #acquire}. +// * +// *

+// * NOTE: it's safe to call this after {@link #close}. +// */ +// public void release(IndexSearcher searcher) throws IOException { +// assert searcher != null; +// searcher.getIndexReader().decRef(); +// } - /** - * Release the searcher previously obtained with {@link #acquire}. - * - *

- * NOTE: it's safe to call this after {@link #close}. - */ - public void release(IndexSearcher searcher) throws IOException { - assert searcher != null; - searcher.getIndexReader().decRef(); - } - - /** - * Close this SearcherManager to future searching. Any searches still in - * process in other threads won't be affected, and they should still call - * {@link #release} after they are done. - */ - public synchronized void close() throws IOException { - if (currentSearcher != null) { - // make sure we can call this more than once - // closeable javadoc says: - // if this is already closed then invoking this method has no effect. - swapSearcher(null); - } - } - - /** - * Obtain the current IndexSearcher. You must match every call to acquire with - * one call to {@link #release}; it's best to do so in a finally clause. - */ - public IndexSearcher acquire() { - IndexSearcher searcher; - do { - if ((searcher = currentSearcher) == null) { - throw new AlreadyClosedException("this SearcherManager is closed"); - } - } while (!searcher.getIndexReader().tryIncRef()); - return searcher; - } - - private void ensureOpen() { - if (currentSearcher == null) { - throw new AlreadyClosedException("this SearcherManager is closed"); - } - } - - private synchronized void swapSearcher(IndexSearcher newSearcher) throws IOException { - ensureOpen(); - final IndexSearcher oldSearcher = currentSearcher; - currentSearcher = newSearcher; - release(oldSearcher); - } +// /** +// * Close this SearcherManager to future searching. Any searches still in +// * process in other threads won't be affected, and they should still call +// * {@link #release} after they are done. +// */ +// public synchronized void close() throws IOException { +// if (currentSearcher != null) { +// // make sure we can call this more than once +// // closeable javadoc says: +// // if this is already closed then invoking this method has no effect. +// swapSearcher(null); +// } +// } +// +// /** +// * Obtain the current IndexSearcher. You must match every call to acquire with +// * one call to {@link #release}; it's best to do so in a finally clause. +// */ +// public IndexSearcher acquire() { +// IndexSearcher searcher; +// do { +// if ((searcher = currentSearcher) == null) { +// throw new AlreadyClosedException("this SearcherManager is closed"); +// } +// } while (!searcher.getIndexReader().tryIncRef()); +// return searcher; +// } +// +// private void ensureOpen() { +// if (currentSearcher == null) { +// throw new AlreadyClosedException("this SearcherManager is closed"); +// } +// } +// +// private synchronized void swapSearcher(IndexSearcher newSearcher) throws IOException { +// ensureOpen(); +// final IndexSearcher oldSearcher = currentSearcher; +// currentSearcher = newSearcher; +// release(oldSearcher); +// } + } Index: lucene/core/src/java/org/apache/lucene/search/NRTManager.java =================================================================== --- lucene/core/src/java/org/apache/lucene/search/NRTManager.java (revision 1241912) +++ lucene/core/src/java/org/apache/lucene/search/NRTManager.java (working copy) @@ -305,7 +305,7 @@ return false; } if (!(setSearchGen = reference.manager.isSearcherCurrent())) { - setSearchGen = reference.manager.maybeReopen(); + setSearchGen = reference.manager.maybeRefresh(); } if (setSearchGen) { reference.generation = newSearcherGen;// update searcher gen