[infinispan-dev] CR3: async stores, again

Thu Aug 26 08:01:18 EDT 2010

On 26 Aug 2010, at 12:22, Sanne Grinovero wrote:

> Hi manik, thanks for reading through my long mail.
> 
> though I didn't understand what you think I should do now :)
> Regarding to "Like I said we should still support commit on a separate thread.",
> you mean my approach is not correct?
> But then you say "Definitely" relating to my question if this is an improvement.

I'm not sure how your approach prevents a commit on a separate thread.  :)

> BTW why should you support "commit" on a separate thread, if all what
> the commit does is move a single value from one concurrent map to
> another one? the heavy work of sending the data down to the real store
> is performed in the separate thread.

Ah sorry - I think I have misunderstood you.  I thought what you meant was that T1 may call prepare() and T2 may call commit(), imposing restrictions on how your impl may make use of, say, a ThreadLocal.

You are absolutely correct in that the work done in commit() does not need to be taken offline/performed asynchronously.

> If you think this is not going in a good direction, I'll leave this
> alone, otherwise I could try cleaning
> up my current experiments and propose a patch.

No, I think this is definitely a good direction.  :)

> 
> Sanne
> 
> 2010/8/26 Manik Surtani <manik at jboss.org>:
>> Hi Sanne
>> 
>> Thanks for the detailed analysis!
>> 
>> On 26 Aug 2010, at 09:05, Sanne Grinovero wrote:
>> 
>>> After (finally!) being able to isolate the issue with a core-only unit
>>> test I changed the name of the issue to
>>> "AsyncStore fails to save all values when batching"
>>> As:
>>> * it's not related to the JDBC stores but the Async wrapper
>>> * I can only reproduce it while batching
>>> 
>>> I hacked a fix for me, which appears to work fine both with the unit
>>> tests and fix the staging issues in my project, but this changed quite
>>> a bit so we should talk about it:
>>> 
>>> I see the current AsyncStore implementation is "coalescing" all
>>> changes being part of the same transaction during the prepare-commit
>>> phase, and then async-delegating the same prepare-commit to the
>>> decorated store. After that, it might also async-delegate a commit or
>>> a rollback.
>>> So if I understood it well enough, this means that:
>>> 
>>>   a) it's not merging changes from different transactions
>>> 
>>>   b) a commit (or rollback) could be received from the decorated
>>> store implementation with a noticeable delay (a full cycle of
>>> collected changes), and most importantly it seems to me that it might
>>> receive a commit in a different thread than the prepare (is that
>>> legal?).
>> 
>> I believe some TMs do allow prepare-and-commit on different threads.  Definitely prepare-and-rollback.
>> 
>>> So I copied the transaction managing code from AbstractCacheStore (as
>>> AsyncStore doesn't inherit from it) and replaced the prepare, commit
>>> and rollback methods of AbstractCacheStore to the same as the
>>> AbstractCacheStore implementations:
>>> this way uncommitted changes are never inserted in the "state" map
>>> (nor are commit and rollback operations), and it seems that I end up
>>> sending to the delegate store only committed changes;
>>> best part of it is that I'm aggregating in a single "coalesced" state
>>> all changes from all transactions (only committed), this seems to me a
>>> good improvement?
>> 
>> Definitely.
>> 
>>> The code I'm having is not that clean, as it copied several lines of
>>> code from the AbstractCacheStore; to clean it up I think that the
>>> AbstractDelegatingStore should inherit from AbstractCacheStore, and
>>> avoid delegating the methods relating to transactions (in fact
>>> implementing what I did for the AsyncStore for all  decorated stores).
>> 
>> Yes, much cleaner that way.
>> 
>>> Also some unit tests will need fixing, as there are explicit tests
>>> asserting that the commit is done in another thread: could I delete
>>> these?
>> 
>> Like I said we should still support commit on a separate thread.
>> 
>>> Finally, as the delegated store is not being used transactionally
>>> anymore (it would never receive a prepare or commit), wouldn't it make
>>> sense to wrap all blocks of changes in a single transaction? rollback
>>> from a failure in the async store is, as far as I understood, not
>>> supported even with current code.
>> 
>> Yes, true.
>> 
>>> I'm eager to hear you opinions, especially what I would break with this design.
>>> 
>>> Cheers,
>>> Sanne
>>> 
>>> 
>>> 2010/8/25 Sanne Grinovero <sanne.grinovero at gmail.com>:
>>>> 2010/8/25 Manik Surtani <manik at jboss.org>:
>>>>> 
>>>>> On 25 Aug 2010, at 14:48, Sanne Grinovero wrote:
>>>>> 
>>>>>> Hello,
>>>>>> I'm testing CR3, I got it so far that I'm testing the stop-restart
>>>>>> application cycle now, I'm using the JdbcStringBasedCacheStore in
>>>>>> async mode,
>>>>>> and every time I stop the application - which issues a clean Stop() on
>>>>>> the CacheManager, it appears that some values are lost.
>>>>>> This is fixed if I remove the async.
>>>>>> 
>>>>>> Is this expected? I hope not, as this way I can't shutdown the cluster
>>>>>> when using async.
>>>>>> This time I can reproduce it with a UT.
>>>>> 
>>>>> Cool, a unit test would help.  Is this only with the JDBC store?
>>>> 
>>>> I understand this is not expected and created ISPN-618; you can find
>>>> my failing unit test attached to the issue.
>>>> 
>>>> I will try to reproduce the same issue depending on core only to
>>>> understand if it's related to JDBC.
>>>> 
>>>> Sanne
>>>> 
>>>>> 
>>>>> --
>>>>> Manik Surtani
>>>>> manik at jboss.org
>>>>> Lead, Infinispan
>>>>> Lead, JBoss Cache
>>>>> http://www.infinispan.org
>>>>> http://www.jbosscache.org
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> infinispan-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>> 
>>>> 
>>> 
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> 
>> --
>> Manik Surtani
>> manik at jboss.org
>> Lead, Infinispan
>> Lead, JBoss Cache
>> http://www.infinispan.org
>> http://www.jbosscache.org
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> 
> 
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Manik Surtani
manik at jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org