[hibernate-dev] Hibernate Search 421

[Emmanuel Bernard]

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.
>>>>>> 
>>>>>> 
>>> 
>>> 
>