Hi,

In replaceActiveReader() method of SharedReaderProvider.java, we always do
reader = IndexReader.open( directoryProvider.getDirectory() );
in case of new reader & also old reader.

Ideally if outOfDateReader is not null, we should do reader.reopen() instead of opening a reader again.

plz correct me if I am wrong.

private IndexReader replaceActiveReader(IndexReader outOfDateReader, Lock directoryProviderLock, DirectoryProvider directoryProvider, IndexReader[] readers) {
boolean trace = log.isTraceEnabled();
IndexReader oldReader;
boolean closeOldReader = false;
boolean closeOutOfDateReader = false;
IndexReader reader;
/**
* Since out of lock protection, can have multiple readers created in //
* not worse than NotShared and limit the locking time, hence scalability
*/
try {
reader = IndexReader.open( directoryProvider.getDirectory() ); }
catch (IOException e) {
throw new SearchException( "Unable to open Lucene IndexReader", e );
}
directoryProviderLock.lock();
try {
//since not protected by a lock, other ones can have been added
oldReader = activeSearchIndexReaders.put( directoryProvider, reader );
semaphoreIndexReaderLock.lock();
try {
searchIndexReaderSemaphores.put( reader, new ReaderData( 1, directoryProvider ) );
if ( trace ) log.trace( "Semaphore: 1 for " + reader );
if ( outOfDateReader != null ) {
ReaderData readerData = searchIndexReaderSemaphores.get( outOfDateReader );
if ( readerData == null ) {
closeOutOfDateReader = false; //already removed by another prevous thread
}
else if ( readerData.semaphore == 0 ) {
searchIndexReaderSemaphores.remove( outOfDateReader );
closeOutOfDateReader = true;
}
else {
closeOutOfDateReader = false;
}
}

if ( oldReader != null && oldReader != outOfDateReader ) {
ReaderData readerData = searchIndexReaderSemaphores.get( oldReader );
if ( readerData == null ) {
log.warn( "Semaphore should not be null" );
closeOldReader = true; //TODO should be true or false?
}
else if ( readerData.semaphore == 0 ) {
searchIndexReaderSemaphores.remove( oldReader );
closeOldReader = true;
}
else {
closeOldReader = false;
}
}
}
finally {
semaphoreIndexReaderLock.unlock();
}
}
finally {
directoryProviderLock.unlock();
}
if ( closeOutOfDateReader ) {
if ( trace ) log.trace( "Closing out of date IndexReader " + outOfDateReader );
try {
outOfDateReader.close();
}
catch (IOException e) {
clean( new SearchException( "Unable to close Lucene IndexReader", e ), readers );
}
}
if ( closeOldReader ) {
if ( trace ) log.trace( "Closing old IndexReader " + oldReader );
try {
oldReader.close();
}
catch (IOException e) {
clean( new SearchException( "Unable to close Lucene IndexReader", e ), readers );
}
}
return reader;
}

Hi,
cool you're reading the sources!
But lookout, the .reopen() is going to return you the same instance if it was not necessary to reopen, so you'll have to manage the close operation in a different way.

For this reason the SharedReaderProvider is now deprecated, and the default one is SharingBufferReaderProvider: this one is using the reopen semantics, and avoid locking and synchronization until required.

thanks Sanne for your reply.
I thought .reopen() updates the exiting IndexReader with latest changes in the indexer files, it does not reopen the entire IndexReader again.

but SharingBufferReaderProvider is not in hibernate-search-3.0.1.GA.

right, it's there since 3.1; some early betas had it already.

the reopen() gets you a new instance, this is a good feature in a transactional context: until you close the current transaction you'll be using the first IndexReader you got, and it's "view" of the index doesn't ever change: that's how we implement Repeatable Read, among other stuff.

So the change to the reopen() semantic in latest Search is only a performance improvement, the behaviour is the same and the change should be transparent to existing applications.