[hibernate-dev] Re: HSEARCH-178

[Sanne Grinovero]

I've 2 inline answers:

2009/4/14 Emmanuel Bernard <emmanuel at hibernate.org>:
> Read inline
>
> On  Apr 13, 2009, at 17:22, Sanne Grinovero wrote:
>
>> After our chat about the topic I thought that I only needed some minor
>> changes,
>> was quite wrong.
>>
>> I moved the flush listener to the usual FullTextIndexEventListener, using
>> delegation at first as we had agreed. This got me into troubles with
>> the Serialization test of the FullTextEntityManager, I had to adapt
>> EventSourceTransactionContext changing a field to transient and
>> having the code manage the case in which the values are lost by
>> deserialization.
>> After this I removed the delegation moving the code to the
>> FullTextIndexEventListener
>> which resulted in simpler code.
>>
>> You made me think about what was happening in case of an error during
>> the flush processing in the default listener, so I've replaced the Map
>> with a combination
>> of non-static ThreadLocal with a weak reference to the flushing Session,
>> which is checked for == identity to make sure the synch is relevant to
>> the same session,
>> in case of a previously not cleaned-up flush by the same Thread (as in
>> a Map, but I only need
>> the last stored value for the current session, and only for the same
>> thread).
>
> I think I don't like the ThreadLocal approach. What is your reasoning for
> using a thread local variable? What would make it less compelling if we were
> not using a TL?

the two event listeners "on-post-somethingchanged" and "flush" are executed
one after the other right away, atomically from a out-of-hibernate-search point
of view. In the test code if I put breakpoints in both the "register
synch" routing
and "find the synch from the flushlistener" thay both happen only when
the client code is asking to flush.
This made me think the two routines are always being called by the same thread,
and there is no chance to begin doing something else between the first operation
and the second operation: it's not possible that, for example, the Session is
passivated or suspendend (thinking about long running conversations).
The only way to stop the second to happen is having
an exception in the default hibernate flush listener: that's the
reason for the weak
reference to Session.
These thoughts leaded me to think that a concurrent map is overkill, especially
considering that every thread in the application is to access
this map twice per flush: to put in an object, and get it back
immediately after that,
and no thread trying to say anything to the others or wanting to store
it for a longer
time. So I thought the ThreadLocal was the natural answer.
Still the resulting code is ugly, I'm happy to forget my performance
thoughts if we
could get with a cleaner solution.
I've another version ready using a WeakIdentityHashMap, it's also
passing the tests,
and is a bit more readable, but I'm unsure about which is better.
Shall I commit the one using WeakIdentityHashMap instead of ThreadLocal?

>
>>
>> I avoided "static" for this ThreadLocal to have it cleanup at
>> SessionFactory close,
>
> makes sense. The map should be scoped to the event listener.
>
>>
>> and needed to be both "final" and "transient" for respectively
>> concurrency and serializability.
>> I've had to add a customized readObject to recreate the ThreadLocal after
>> a deserialization, using Reflection to set the final field.
>
> That is weird. I thought readObject had the same rights than constructors
> wrt to final assignment.

Yes it's weird; here is a good source about the topic:

http://jeremymanson.blogspot.com/2008/07/immutability-in-java-part-3.html

Short and straight to the point, I'd suggest reading it.
quote: "It is pretty ridiculous to have to use reflection and setAccessible."


>
>>
>> A bit messy, I'm not very satisfied, but it's working fine.
>>
>> I'm committing it to trunk, but would be glad if someone
>> could get an idea to make it a bit cleaner/simpler.
>> Also I'll wait for a "go" before merging it to 3.1 branch
>>
>> Sanne
>>
>>
>> 2009/4/8 Emmanuel Bernard <emmanuel at hibernate.org>:
>>>
>>> Just port it also to trunk and we will see if it's stable.
>>> We will probably carve a 3.1.1 soon and a 3.1.2 if something bad is
>>> happening I guess
>>> On  Apr 8, 2009, at 11:48, Emmanuel Bernard wrote:
>>>
>>>> yes ideally trunk is better suited for experiments
>>>>
>
>