Re: [hibernate-dev] Hibernate Search: Master/Slave DirectoryProviders improvements proposal
Hi Sanne,
A) ... 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.
Is this really required? Is synchronisation not ensured via
SearchFactoryImplementor.getLockableDirectoryProviders()?
B) ... Also I would like to address the use of finalize() to stop the
Timer,
there is a TODO about it already;
Fair point. Initially a non daemon thread was used. Once we switched to a
deamon thread we
could probably have got rid of the call to timer.canel().
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).
Sounds reasonable.
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.
The properties are different. Yes, the master and the slave both define:
hibernate.search.default.sourceBase and
hibernate.search.default.indexBase, but remember
that these are in two different configuration files. Master and slave are
two seperate machines
or at the very least two different JVMs. Nothing stops you to have
different mount points. I guess the
docuemtnation might suggest this.
You can definitely create a Jira issue for B and D. Not sure about A
though.
Maybe someone else can have another look at the code.
--Hardy