[infinispan-dev] CR3: async stores, again

[Manik Surtani]

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