[infinispan-dev] CR3: async stores, again

Sanne Grinovero sanne.grinovero at gmail.com
Thu Aug 26 08:20:08 EDT 2010


2010/8/26 Manik Surtani <manik at jboss.org>:
>
> 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.  :)

because the caller of AsyncStore.prepare(..) - T1 - will have the
modifications list added to the "transactions" juc.ConcurrentHashMap,
using the TX as a key, and this same values will be pushed to "state"
by AsyncStore.commit(..), nothing else is done, the writing to the
decorated store is done by T2 using the current logic which takes
modifications from "state", totally independent and unaware to the
existing transaction used by T1.

>
>> 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.

that's what is currently possible as the Executor in asyncStore might
have >1 threads, on turn taking some of the operations, so some
prepares() could go to thread X and some commits() to thread Y, so it
seems to me that thread X might perform the prepare for TX1 and the
thread Y receive the commit for the same TX1 (they might even try to
perform them in inverted order).

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

Ok

>
>> 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.  :)

Ok, I'll try cleaning up and send you a patch. I assume this should go
in 4.1.0.Final, so we should handle it with care.. please continue
supporting me as this code is all new for me, I just make assumptions
on some parts of it.

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
>
>
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>



More information about the infinispan-dev mailing list