[hibernate-dev] Hibernate Search 421

[Amin Mohammed-Coleman]

Hi Emmanuel

Thanks for your input. I've just had the eureka moment which I was  
struggling with. Please give me a day or two and create a patch that  
conforms to your thoughts

Thanks for your patience!

Cheers

Amin

Sent from my iPhone

On 22 Mar 2010, at 17:43, Emmanuel Bernard <emmanuel at hibernate.org>  
wrote:

> Hello,
> Sorry, I've been a bit out of this particular problem for a long  
> time, so if I ask already answers questions forgive me.
>
> Thanks for all the work, sorry it take so long on our part for each  
> review :(
>
> Here are some comments on the patch:
> - your code rules don't seem to match Hibernate's in particular you  
> use import com.acme.*; which we don't. It makes the patch less  
> readable for us as many areas are changes wo need.
> - getSyncBackendExceptionHandler / getAsyncBackendExceptionHandler  
> do a lot of init. Are they called several times? I'd rather see them  
> named build* instead of get*
> - don't use log.warn when creating the exception handler. Either do  
> nothing or raise an exception. In this case I'd favor raising a  
> SearchException
> - this has been discussed I think but do we need an async and a sync  
> exception handler? Or do we need an exception handler? What's the  
> rational for the split? If a split is necessary, what's the rational  
> for two different APIs?
> - I don't like classes named Default*, they should have a meaningful  
> name and purpose somehow: LogExceptionHandler,  
> RethrowExceptionHandler etc
> - Why exactly do we not return anything during the sync and return  
> Thread.UncaughtExceptionHandler during the async? Couldn't we write  
> adaptors to hide the complexity to the user if some transformation  
> is needed?
> - we should pass more data to the exception. I imagine in many cases  
> we know what entity is being indexed (type and id), if we create or  
> delete the index. There are useful info to rerun the indexing  
> process or analyze what's going on. Can we imagine an API for that?  
> Something like:
>
> class ErrorHandler {
>    void handle(ErrorContext context);
> }
>
> interface ErrorContext {
>  //TODO list or set?
>  Set<FailingOperation> getFailingOperations();
>  FailingOperation getOperationAtFault();
>  //TODO should it be Throwable or something more specific like  
> SearchException?
>  Throwable getThrownException();
> }
>
> intarface FailingOperation {
>  Object getId(); //is it Serializable?
>  Class<?> getEntityClass();
>  Operation getOperation();
> }
>
> WDYT?
>
> On 19 mars 2010, at 20:55, Amin Mohammed-Coleman wrote:
>
>>
>> Hi
>>
>> Please find updated patch with the removal of the enum singleton.
>>
>> Thanks
>> Amin<HibernateSearch421-19032010.patch>
>> On 17 Mar 2010, at 12:58, Sanne Grinovero wrote:
>>
>>> Hi Amin,
>>> thanks for the update, see some thoughts:
>>>
>>> 2010/3/16 Amin Mohammed-Coleman <aminmc at gmail.com>:
>>>> Hi folks,
>>>>
>>>> I've removed the enum singleton and created a class 
>>>> (BackendExceptionHandler) which has 2 methods:
>>>>
>>>>
>>>> public Thread.UncaughtExceptionHandler getUncaughtExceptionHandler 
>>>> (SearchConfiguration configuration);
>>>>
>>>> public boolean logException(SearchConfiguration configuration)
>>>
>>> Care to explain how I should use them? Are we not going to have a
>>> common interface? In that case does it make sense to have a method
>>> named "logException", which would imply a logging implementation?
>>>
>>>>
>>>> The issue that I'm looking at is getting the search configuration  
>>>> to the methods.  In order to get the SearchConfiguration to the  
>>>> methods defined in BackendExceptionHandler, I have to thread  
>>>> SearchFactoryImplementor.
>>>>
>>>> Using this approach I will have to define a method to get the  
>>>> search configuration from the SearchFactoryImplementor.  I'm  
>>>> guess this isn't the best approach as this requires a significant  
>>>> change.  I don't know what peoples thoughts are on this.  I'm  
>>>> looking to set the BackendExceptionHandler up when the  
>>>> SearchFactory is created and then use it.  Is there any currently  
>>>> approach that does something similar?
>>>
>>> Ah there's a subtle problem with that, which is that we can't hold a
>>> reference in Search to a Configuration at runtime: use it at
>>> SearchFactory creation, extract all what you need, but then you have
>>> to clear references or we're going to have a memory leak. [1]
>>> The solution is, as we do with all components, to start them during
>>> SearchFactory initialization and then expose a getter to initialized
>>> service.
>>>
>>> [1] http://opensource.atlassian.com/projects/hibernate/browse/HSEARCH-314
>>>
>>> Cheers and thanks for the complex work,
>>> Sanne
>>>
>>>>
>>>>
>>>> Thanks for your patience with this one.
>>>>
>>>> Cheers
>>>> Amin
>>>>
>>>>
>>>>
>>>>
>>>> On 9 Mar 2010, at 13:00, Amin Mohammed-Coleman wrote:
>>>>
>>>>> Hi Sanne
>>>>>
>>>>> You are right and Im not happy with the enum class. I wanted to  
>>>>> have a single configuration that was available on the creation  
>>>>> of the search factory and re use when required.  I'll take a  
>>>>> look at changing that with a better solution.
>>>>>
>>>>> Cheers
>>>>>
>>>>> Amin
>>>>>
>>>>> Sent from my iPhone
>>>>>
>>>>> On 9 Mar 2010, at 12:47, Sanne Grinovero <sanne.grinovero at gmail.com 
>>>>> > wrote:
>>>>>
>>>>>> yes that is it;
>>>>>> There has been some talking about other strategies as well on  
>>>>>> previous
>>>>>> mails, like jms, but that lead to nowhere so yes I'm suggesting  
>>>>>> now to
>>>>>> forget about other default implementations at the moment and  
>>>>>> proceed
>>>>>> as you just said.
>>>>>>
>>>>>> 2010/3/9 Emmanuel Bernard <emmanuel at hibernate.org>:
>>>>>>> I thought the goal was to have something pluggable with two  
>>>>>>> default impls (exception and log). If that's what you are  
>>>>>>> describing I am ok, if not, then I don't understand ;)
>>>>>>>
>>>>>>> On 9 mars 2010, at 12:08, Sanne Grinovero wrote:
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>
>>>>
>>
>