[hibernate-dev] Hibernate Search 421

Sun Mar 28 18:15:26 EDT 2010

Ok I got the patch and agree this is now going in the good direction.
This errorhandling should be kept very simple, remember that if an
error happens there they will be swallowed and hidden - some logging
is all I asked for.
Also we're not always capable of telling which exact operation failed;
this might be nice when we can, but keep all context information
optional.
Is ErrorContext needing to be an interface? I don't see that
implementation easily replaceable but I might have missed what you're
planning to do next.

Now the error should be handled in a try/catch block around
org.hibernate.search.backend.impl.lucene.LuceneBackendQueueProcessor
and
org.hibernate.search.backend.impl.lucene.PerDPQueueProcessor

Are we still intentioned to provide an implementation to rethrow the
exception back to the same thread which was committing the
transaction?
that would need some changes also in
org.hibernate.search.backend.impl.lucene.QueueProcessors

Regards,
Sanne

2010/3/28 Sanne Grinovero <sanne.grinovero at gmail.com>:
> Hi Amin,
> I couldn't find your patch, could you attach it to the issue?
> I don't think this mail list accepts attachments.
>
> Cheers,
> Sanne
>
> 2010/3/25 Amin Mohammed-Coleman <aminmc at gmail.com>:
>> Hi
>>
>> I have created a patch based on Emmanuel's thoughts.  I haven't applied the usage of the ErrorHandler yet as I wanted to get an idea on whether it was the right path.  I have mentioned this before but I'm not sure how to capture the operation at fault followed by subsequent failures.  I'd appreciate it if someone could give me a pointer.
>>
>> I think I spoke to Sanne about this and the approach was to capture each of the exceptions and log it (or configured by the user), but the error context doesn't fit with this approach.
>>
>>
>>
>> Thanks
>> Amin
>>
>>
>>
>>
>>
>>
>> On 22 Mar 2010, at 17:43, Emmanuel Bernard 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>
>>
>>
>> _______________________________________________
>> hibernate-dev mailing list
>> hibernate-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>