[hibernate-dev] Re: HSEARCH-178

I've 2 inline answers: 2009/4/14 Emmanuel Bernard <emmanuel(a)hibernate.org&gt;: > 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?