[hibernate-dev] Re: Hibernate Search: Master/Slave DirectoryProviders improvements proposal

Hello most esteemed developers, I have been scrutinizing the FSMasterDirectoryProvider and FSSlaveDirectoryProvider classes in Hibernate Search and have found some minor glitches and space for some improvement (IMHO); I would like to hear your opinion about it, and if you agree I'll open a JIRA and provide a patch myself ASAP. A) Variable visibility both FSMasterD.P. and FSSlaveD.P. have some little concurrency problems: there is a "volatile boolean inProgress" that fixes the communication beetween the "CopyDirectory" Runnable and the "TriggerTask" Timer, but all comunication beetween the client thread calling "getDirectory()" and the CopyDirectory which defines the current directory is unprotected; also volatile isn't enough to fix it, IMHO an AtomicReference would be more appropriate. The locking schema looks great! I've had a hard time understanding the flow and did some tests about the behaviour of Lucene in case of concurrent index-deletion and forcing the most dangerous situations I could think of, I couldn't find anything failing. Actually in the current situation the lock works ok but the wrong Directory could be returned because of the visibility problems; the lock avoids most problems but could hurt scalability (as we could return locked resources even whenever an unlocked dir was available).

B) The finalize / stop issue Also I would like to address the use of finalize() to stop the Timer, there is a TODO about it already; IMHO the best solution is to remove the finalize without changing other code: the Timer is declared as daemon thread already, so it's going to stop automatically on JVM exit.

The current finalize idea could harm the GC, as in the finalize we are referencing the timer and instruct the timer to do something: the timer has a reference to <this> so this would revive the reference to <this> and cancel the garbage-collection; actually I don't think this finalizer is ever being called, as the timer also has a reference to this (being an inner class) and is running in another thread which is certainly alive (as our purpose was to stop it and we we still didn't get to that point). So we could fix this issues just removing code, and making some instance variables (such as the Timer itself) unnecessary. Also this means you won't need a stop() to be added to DirectoryProvider interface, at least not for FSMasterD.P. and FSSlaveD.P. ( which are the only known implementations needing it, so we could also remove the TODO in the DirectoryProvider. C) FileHelper not listening to Thread.interrupt() requests As FileHelper.synchronize is used in the mentioned Timers to copy potentially big files, it would be nice to check for Thread.isInterrupted() and also catch ClosedByInterruptException in file-copy operations so to abort all I/O operations and not just the current one to abort faster on JVM shutdown. (It doesn't look like the copy safety was guaranteed anyway).

D) Master and Slave share configuration properties. Wouldn't it be natural to have two different property keys to manage the shared index path? I imagine the typical scenario would be to have the two configured on different servers and copy through some NFS, but it looks like we are forcing people to use exactly the same path on both machines as the two classes read the same props.