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(a)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(a)gmail.com>:
>>> 2010/8/25 Manik Surtani <manik(a)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(a)jboss.org
>>>> Lead, Infinispan
>>>> Lead, JBoss Cache
>>>>
http://www.infinispan.org
>>>>
http://www.jbosscache.org
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> infinispan-dev(a)lists.jboss.org
>>>>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev(a)lists.jboss.org
>>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> --
> Manik Surtani
> manik(a)jboss.org
> Lead, Infinispan
> Lead, JBoss Cache
>
http://www.infinispan.org
>
http://www.jbosscache.org
>
>
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev(a)lists.jboss.org
>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org