[hibernate-dev] Hibernate Search 421

[Sanne Grinovero]

Hi Amin,
thanks to you, and sorry for the delay.
I've committed a first step that you obviously need - indipendently
from how we manage it - please see:
http://fisheye.jboss.org/changelog/Hibernate/search?cs=18939

So this should simplify your patch having impact only on the exception
handling strategy.
I have to ask you one more change: could you avoid this enum? It
doesn't feel right to use this hack to achieve a singleton, and
storing state in it doesn't make me feel right: you could have more
than one Search application configured in different ways and these are
going to affect each other when using an enum configuration. Besides
that, you would enter in thread visibility issues which is a pain to
manage (the initialization of the enum would need synchronization and
other issues like that).

Emmanuel are you ok with this if we either log or rethrow the
exception back to application code? jms et al looks complex and I
don't see the benefit.

Sanne


2010/3/1 Amin Mohammed-Coleman <aminmc at gmail.com>:
> Hi Sanne
>
> Thanks for the feedback.  Please find attached the updated patch with your initial recommendations. Sorry about the formatting issues with the original patch.
>
>
> Cheers
> Amin
>
>
>
> On 28 Feb 2010, at 23:11, Sanne Grinovero wrote:
>
>> Hi Amin,
>> thanks for the patch; it's a bit late now so I'll look into the
>> details in the next days; some early feedback:
>>
>> 1)Could you update the patch to avoid whitespace reformatting changes?
>> It looks like your IDE reformatted some methods and import statements
>> which appear unchanged code, this makes it a bit hard to review.
>>
>> 2)About the code in SearchFactoryImpl, make sure the line
>> "initialiseBackendExceptionConfiguration(cfg);"
>> happens before the write to the barrier (I mean: replace positions
>> with the line above it, the code "this.barrier = 1;" should be the
>> last one in the method).
>>
>> 3)Executors.newFixedThreadPool:
>> if it's a fixedThreadPool the min and max should always be the same
>> value, so either use just one parameter or change the method name.
>>
>> 4) You're loading the custom strategy by reflection in
>> BackendExceptionHandler; that's fine but if you could use the
>> org.hibernate.search.util.PluginLoader utility you'd be sure to use
>> the right classloader and benefit from improved error messages in case
>> of exceptions.
>> Method signature is scary but look at other usages for some examples.
>>
>> thank you for all the help,
>> Sanne
>>
>> 2010/2/25 Amin Mohammed-Coleman <aminmc at gmail.com>:
>>> Hi Guys
>>>
>>> Sorry for the delay with this. I've been pretty tied up with the new job and also tried looking at doing this from various angles.  Please find attached a patch which is more of a POC and I wanted to get your thoughts (and criticisms!).  The coding is more in line with Sanne's recommendation that is because I tried looking at implementing something that you suggested Emmanuel but I'm afraid I got stuck.
>>>
>>> You'll come across a class BackendExceptionHandler which I'm not very happy about but I wanted to get something together to get a discussion.
>>>
>>> Again apologies for the delay.
>>>
>>>
>>> Cheers
>>> Amin
>
>
>