[hibernate-dev] [HSearch] Configuration properties

[Emmanuel Bernard]

On 12 juil. 2010, at 15:43, Hardy Ferentschik wrote:

> On Mon, 12 Jul 2010 15:31:17 +0200, Emmanuel Bernard <emmanuel at hibernate.org> wrote:
> 
>>> just wondering whether there is a reason why we don't expose the
>>> configuration properties in SearchFactory?
>>> I think we talked about this once, but I cannot remember exactly.
>> 
>> There are several reasons but one of them is to know how things are used and how instead in a more type-safe way.
> 
> Personally I don't see a problem of having access to the properties. Not having
> access on the other hand can make code more complicated than it has to be.
> 
> Let's take MassIndexerImpl for example. This class instantiates a SimpleIndexingProgressMonitor
> at the moment. It also gets passed the SearchFactoryImplementor. If the SearchFactoryImplementor
> would give me access to the properties I could just check hibernate.search.jmx_enabled and
> act decide whether I want to create a JMXIndexingProgressMonitor. Or maybe we want to
> make the progress montior configurable via hibernate.search.progress_monitor_class.
> Would you prefer to keep adding methods to SearchFactoryImplementor?
> 

Today in the code, here is the de facto rule (I think this is consistent ATM):
 - stuffs created at SF init time can access the properties
 - otherwise, the required state is exposed via the SearchFactoryImplementor

That way:
 - know how things are used and how instead in a more type-safe way
 - you avoid the redundant property look up (which synchronizes) and parsing

If we start mixing stuffs the code will become less readable.

Now about the JMXIndexingProgressMonitor:
 - is there one published for all mass indexers run in parallel?
 - or is there one per mass indexer
 - if the latter how will you get the list?

Maybe a 
SFImplementor#getIndexingProgressMonitor(...) or SFImplementor#getIndexingProgressMonitorFactory(...) would be a better contract?

> 
>>> I noticed that the properties are now exposed in
>>> StateSearchFactoryImplementor anyways. Maybe
>>> we could push this all the way down to SearchFactory?
>> 
>> The reason it's in StateSearchFactoryImplementor is for people *not* to access it unless they are in need to clone a SearchFactory
> 
> Is this not encouraging things like:
> 
> if ( searchFactory instanceof StateSearchFactoryImplementor ) {
> 	StateSearchFactoryImplementor stateSearchFactoryImplementor = ( StateSearchFactoryImplementor ) searchFactory;
> 	...
> }

Typically people that do (SearchFactoryImpl) sfi today are on for a big surprise for the next release :)
Note that I've put it in the spi.internals package to make sure people know they are shooting themselves in the foot.

> 
> 
>>> While looking at the code I also started to wonder whether we need all
>>> these sub-interfaces for SearchFactory. Currently we have
>>> SearchFactory -> SearchFactoryIntegrator -> SearchFactoryImplementor ->
>>> StateSearchFactoryImplementor
>> 
>> SearchFactory is for app developers
>> SearchFactoryIntegrator is for frameworks like Infinispan (but is still a work in progress, we might end up merging it back with SearchFactoryInplementor)
>> SearchFactoryInplementor is for components of HSearch to access metadata
>> 
>>> -> Mutable-/Immutable-SearchFactory
>> 
>> These are implementations and not exposed.
> 
> Ok, merging SearchFactoryIntegrator into SearchFactoryInplementor seems to be a step forward.
> When you say "work in progress" - what is the status on the methods defined in
> SearchFactoryIntegrator?

Well I realized that there are really 2 contracts:
 - one between SearchFactory and frameworks it integrates like Hibernate Core and Infinispan
 - one between SearchFactory and the internals of Hibernate Search

The goal was to not have much problem changing the latter but be more cautious about breaking the former. That being said, I could not make SearchFactoryInplementor disappear from some other SPIs esp for people implementing back ends.
My main goal is to limit contract breaks between HSearch and Infinispan at this stage and a dedicated interface looked like a good way to do it.