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?

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

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

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?

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

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?

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.

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&gt;: > 2010/8/25 Manik Surtani <manik(a)jboss.org&gt;: >> >> 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

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.

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.

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.

Sanne 2010/8/26 Manik Surtani <manik(a)jboss.org&gt;: > 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&gt;: >>> 2010/8/25 Manik Surtani <manik(a)jboss.org&gt;: >>>> >>>> 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

2010/8/26 Manik Surtani <manik(a)jboss.org&gt;: > > 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(a)jboss.org&gt;: >>> 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&gt;: >>>>> 2010/8/25 Manik Surtani <manik(a)jboss.org&gt;: >>>>>> >>>>>> 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 > > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev >

From the Store's perspective all operations are allowed to be

Taking in account how complex this is getting, we should consider what I mentioned in https://jira.jboss.org/browse/ISPN-329. The use of multiple worker threads to consume async messages and sent them to the store is used primarily to speed up sending those messages to the store. Trying to use coalescing here is not that useful, because the aim is sending the modifications as quickly as possible to the store. So, the chances of having multiple mods for same key are small. On the other hand, if you used a scheduled async store where you send modifications periodically, using a single worker thread makes more sense and coalescing here is key to avoid sending unnecessary modifications. Now, if we only use coalescing when there's a single worker thread, we'd avoid a lot of headaches below since there would not be reordering issues between diff worker threads. So, I think we should revert back the current async store to a multi worker, no coalescing async store, or what I call Unscheduled Write-Behind strategy. When we look at scheduled async store, we'd put back coalescing. For reference, please find attached an email I sent to Manik where I discussed the differences between scheduled and unscheduled async stores and the wiki http://community.jboss.org/wiki/Write-ThroughAndWrite-BehindCaching: On Aug 27, 2010, at 8:37 PM, Sanne Grinovero wrote: > Some more news about this: > Mircea attached a patch he was working on to JIRA, but he hadn't time > to fix all tests, so I'm continuing. > > There are two more issues: > > 1)keeping an ordered list of transactions works fine for transactions > only, but modifications done in a transaction are wrongly reordered > with operations not being done in a transaction. > > 2) Even in the committed current code, I see that a clear() operation > is sent async together with all other operations. Then, when the state > changes are performed, the clear() causes the previously found > operations to be canceled, but "previous" applies here on the iterator > on the state map, not on the order of application of those state > Modifications, so the clear() is removing random Modifications. > > 3) Also my attempt to reuse some code from AbstractCacheStore though > inheritance is not going to make it as this implementation is doing > too much for the needs of AsyncStore: it cares for async Purge, and > all methods I wanted to commit are slightly different because of that. > > Proposal: > 1- Out-of-transaction modifications are pushed into a BlockingDeque > instead of a Map. > > 2- A prepare(tx, list) sets the list aside in the "transactions" Map. > > 3- A commit(tx) takes the list of operations from the "transactions" > and pushes the list on the same BlockingDeque as all other operations, > guaranteeing same order of application. > > 4- a clear operation clears the deque and pushes a Clear() operation on it > > 5- a purge operation is sent though the same deque: only risk I see is > being removed by a clear, but I bet there won't be much to purge after > that :) Also we don't care if the purge is going to be handled async > in the other store, that's a store implementation detail from what I > see in AbstractCacheStore. > > 6- A single processor of AsyncStore drains blocks of a fixed > configurable amount, puts them in a local map, applies the coalescing > algorithm, and puts the results on a new state map. > It seems to me that this worker needs to be exactly one, or reordering > problems arise again in the order of the blocks taken from the deque. > > 6/a - When receiving a clear() - exceptional case - we clear the map > of operations, and issue the clear() synchronously: not adding > anything to the map, and aquiring a write lock "clearop" to avoid race > conditions with the workers which might already be busy but happen to > finish after the clear(). > > 6/b - When receiving a purge() this bypasses other logic and is > directly passed to the decorated Store. > > 7- Now here it becomes tricky, we removed the transactions problem, > the clear operation and the purge from the table. But there still is a > map of Modifications to be applied, by N threads, and to which another > thread might write. This looks like the original problem in simplified > form, but reorderings are still possible as worker A might get some > work on key K1, then worker B kicks in, gets new work about K1 and > happens to perform it before A gets priority again. > Clearly this must be performed either by one thread or using very fine > grained locking: as we're only interested on a per-key lock, we don't > care on the order of operations on other keys: we could use a second > map to store the keys which "are being processed": when a worker takes > some work it uses atomic operations to check if somebody is working on > the same stuff, if it is it could either wait or put it back (if it > wasn't changed, otherwise discard) and pick another work to do. > > Comments? > > AFAIR the issue I mention on point 7 is a known limitation of Async, > so I could implement that in future as an improvement? I would be fine > in having just one worker. > > I wish I had time to do it all soon :| > Sanne > > > 2010/8/26 Sanne Grinovero <sanne.grinovero(a)gmail.com&gt;: >> 2010/8/26 Manik Surtani <manik(a)jboss.org&gt;: >>> >>> 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(a)jboss.org&gt;: >>>>> 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&gt;: >>>>>>> 2010/8/25 Manik Surtani <manik(a)jboss.org&gt;: >>>>>>>> >>>>>>>> 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 >>> >>> >>> >>> >>> >>> _______________________________________________ >>> 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 -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 27 Aug 2010, at 19:37, Sanne Grinovero wrote: > Some more news about this: > Mircea attached a patch he was working on to JIRA, but he hadn't time > to fix all tests, so I'm continuing. > > There are two more issues: > > 1)keeping an ordered list of transactions works fine for transactions > only, but modifications done in a transaction are wrongly reordered > with operations not being done in a transaction. ah I see. The operations within a transaction do not get reordered though, right?

> > 2) Even in the committed current code, I see that a clear() operation > is sent async together with all other operations. Then, when the state > changes are performed, the clear() causes the previously found > operations to be canceled, but "previous" applies here on the iterator > on the state map, not on the order of application of those state > Modifications, so the clear() is removing random Modifications. yes > > 3) Also my attempt to reuse some code from AbstractCacheStore though > inheritance is not going to make it as this implementation is doing > too much for the needs of AsyncStore: it cares for async Purge, and > all methods I wanted to commit are slightly different because of that. > > Proposal: > 1- Out-of-transaction modifications are pushed into a BlockingDeque > instead of a Map. do we need a deque  for this? Also do we want it blocking? Perhaps we can make this configurable and operate on an interface directly?

> > 2- A prepare(tx, list) sets the list aside in the "transactions" Map. > > 3- A commit(tx) takes the list of operations from the "transactions" > and pushes the list on the same BlockingDeque as all other operations, > guaranteeing same order of application. looks good > > 4- a clear operation clears the deque and pushes a Clear() operation on it > > 5- a purge operation is sent though the same deque: only risk I see is > being removed by a clear, but I bet there won't be much to purge after > that :) Also we don't care if the purge is going to be handled async > in the other store, that's a store implementation detail from what I > see in AbstractCacheStore.

> > 6- A single processor of AsyncStore drains blocks of a fixed > configurable amount, puts them in a local map, applies the coalescing > algorithm, and puts the results on a new state map. > It seems to me that this worker needs to be exactly one, or reordering > problems arise again in the order of the blocks taken from the deque. +1

> > 6/a - When receiving a clear() - exceptional case - we clear the map > of operations, and issue the clear() synchronously: not adding > anything to the map, and aquiring a write lock "clearop" to avoid race > conditions with the workers which might already be busy but happen to > finish after the clear(). aren't we using only one worker as per the previous paragraph?

> > 6/b - When receiving a purge() this bypasses other logic and is > directly passed to the decorated Store. +1 > > 7- Now here it becomes tricky, we removed the transactions problem, > the clear operation and the purge from the table. But there still is a > map of Modifications to be applied, by N threads, and to which another > thread might write. This looks like the original problem in simplified > form, but reorderings are still possible as worker A might get some > work on key K1, then worker B kicks in, gets new work about K1 and > happens to perform it before A gets priority again. > Clearly this must be performed either by one thread or using very fine > grained locking: as we're only interested on a per-key lock, we don't > care on the order of operations on other keys: we could use a second > map to store the keys which "are being processed": when a worker takes > some work it uses atomic operations to check if somebody is working on > the same stuff, if it is it could either wait or put it back (if it > wasn't changed, otherwise discard) and pick another work to do. > It might be good to use the same approach as with async marshalling: use one thread per default which would guarantee ordering. Allow a user to specify more that one thread, and clearly explain that reordering might happen: there might be situations where one would not care about ordering after all.

One more thing I've just noticed: AsyncStore processes Commit but doesn't do the same with Rollback. On rollback it should remove the tx from the transaction(as described at 2) ) map to avoid memory leaks.

On 31 Aug 2010, at 11:57, Sanne Grinovero wrote: > 2010/8/31 Mircea Markus <mircea.markus(a)jboss.com&gt;: >> >> On 27 Aug 2010, at 19:37, Sanne Grinovero wrote: >> >>> Some more news about this: >>> Mircea attached a patch he was working on to JIRA, but he hadn't time >>> to fix all tests, so I'm continuing. >>> >>> There are two more issues: >>> >>> 1)keeping an ordered list of transactions works fine for transactions >>> only, but modifications done in a transaction are wrongly reordered >>> with operations not being done in a transaction. >> ah I see. The operations within a transaction do not get reordered though, right? > > That's right, if I only use transactions it works as a charm, ignoring > the corner-case of the clear() operation as explained in next > paragraph. > The problem with Lucene (to show a use case) is that it sometimes > starts a batch for performance reasons; other one-shot operations are > missing the batch. > >> >>> >>> 2) Even in the committed current code, I see that a clear() operation >>> is sent async together with all other operations. Then, when the state >>> changes are performed, the clear() causes the previously found >>> operations to be canceled, but "previous" applies here on the iterator >>> on the state map, not on the order of application of those state >>> Modifications, so the clear() is removing random Modifications. >> yes >>> >>> 3) Also my attempt to reuse some code from AbstractCacheStore though >>> inheritance is not going to make it as this implementation is doing >>> too much for the needs of AsyncStore: it cares for async Purge, and >>> all methods I wanted to commit are slightly different because of that. >>> >>> Proposal: >>> 1- Out-of-transaction modifications are pushed into a BlockingDeque >>> instead of a Map. >> do we need a deque  for this? Also do we want it blocking? Perhaps we can make this configurable and operate on an interface directly? > > any kind of order-mantaining will do, I did it with an unbounded > blocking deque as that's nice to wait on for new work to appear, to > avoid starvation and provide a clean road for shutdown. > >>> >>> 2- A prepare(tx, list) sets the list aside in the "transactions" Map. >>> >>> 3- A commit(tx) takes the list of operations from the "transactions" >>> and pushes the list on the same BlockingDeque as all other operations, >>> guaranteeing same order of application. >> looks good >>> >>> 4- a clear operation clears the deque and pushes a Clear() operation on it >>> >>> 5- a purge operation is sent though the same deque: only risk I see is >>> being removed by a clear, but I bet there won't be much to purge after >>> that :) Also we don't care if the purge is going to be handled async >>> in the other store, that's a store implementation detail from what I >>> see in AbstractCacheStore. > Not sure we need to process purge in an async way: it is already > called asynchronously by the eviction thread, so we can just delegate > the call to the underlaying call. > Right, I got inspired by previous version. I have it done now by the > AsyncStoreCoordinator, not spawning additional AsyncProcessors, as > it's assumed to be async and return quickly; this way this kind of > operation gets a sort of "priority" towards other modifications, but > sill async from the point of view of the caller. The caller of purge is the eviction thread which is again an async thread (not a user thread). So there would be two levels of async calls. The only way in which a user can call purge is through the AdvancedCache.getEvictionManager().processEviction() - but I think if one does so it would want to have purging executed sync.

>> > > +1, changing that. > > Now I'm in need of feedback about adding the "shutdown timeout" option > to AsyncStoreConfig; > and what to set as default? Yeah I think a config option makes sense. I presume this timeout is a "max wait time", not a "guaranteed wait time"? If the former, a default of 1h or something large, to make sure writes are flushed would be a sensible default.

2010/8/31 Mircea Markus <mircea.markus(a)jboss.com&gt;: > > On 27 Aug 2010, at 19:37, Sanne Grinovero wrote: >> >> 3) Also my attempt to reuse some code from AbstractCacheStore though >> inheritance is not going to make it as this implementation is doing >> too much for the needs of AsyncStore: it cares for async Purge, and >> all methods I wanted to commit are slightly different because of that. >> >> Proposal: >> 1- Out-of-transaction modifications are pushed into a BlockingDeque >> instead of a Map. > do we need a deque for this? Also do we want it blocking? Perhaps we can make this configurable and operate on an interface directly? any kind of order-mantaining will do, I did it with an unbounded blocking deque as that's nice to wait on for new work to appear, to avoid starvation and provide a clean road for shutdown.