Pedro’s suggestion looks good [1] It’s passed tests both locally (20 threads, each sending 200 operations) and in a remote server (40 threads, each sending 1000 operations).
Cheers, [1] https://github.com/infinispan/infinispan/pull/2671 On 25 Jun 2014, at 10:21, Galder Zamarreño <[email protected]> wrote: > > On 24 Jun 2014, at 19:51, Pedro Ruivo <[email protected]> wrote: > >> >> >> On 06/24/2014 05:11 PM, Mircea Markus wrote: >>> >>> On Jun 24, 2014, at 16:50, Galder Zamarreño <[email protected]> wrote: >>> >>>> >>>> On 24 Jun 2014, at 16:51, Mircea Markus <[email protected]> wrote: >>>> >>>>> >>>>> On Jun 24, 2014, at 15:27, Galder Zamarreño <[email protected]> wrote: >>>>> >>>>>> To fix this, I’ve been working with Dan on some solutions and we’ve >>>>>> taken inspiration of the new requirements appearing as a result of >>>>>> ISPN-2956. To be able to deal with partial application of conditional >>>>>> operations properly, transactional caches are needed. So, the solution >>>>>> that can be seen in [4] takes that, and creates a transaction around the >>>>>> replaceIfUmodified and forces the getCacheEntry() call to acquire lock >>>>>> via FORCE_WRITE_LOCK flag. >>>>> >>>>> so the entire underlaying cache needs to be transactional then? >>>> >>>> Yeah, it needs to be transactional, but the code I’ve written also deals >>>> with the fact that the cache might not be transactional. I’ll probably add >>>> a WARN message when it’s not transactional. This goes in hand with the >>>> recommendations for ISPN-2956, whereby failover for conditional operations >>>> relying on return values require transactional caches to properly deal >>>> with failover situations. >>>> >>>> To sum up, if using conditional operations or CRUD methods with >>>> Flag.FORCE_RETURN_VALUE, caches should be transactional. Moreover, to >>>> achieve concurrency guarantees of counter tests such as the one tested for >>>> 4424, locking needs to be pessimistic too. If not using conditional >>>> operations or CRUD methods without the flag, the cache could be >>>> non-transactional. >>> >>> Thanks for the analysis. I think we should go with your patch for ISPN 7.0 >>> and consider the proper solution for the future, as you suggest below. >>> +1 for the warning, users should be made aware for the limitation. >> >> +1 for the patch. >> >> Another suggestion: in *InternalEntryFactory.update()*, you can >> synchronize in the cache entry, and create a new method >> *InternalCacheEntry copy(InternalCacheEntry)* that makes a copy while it >> also synchronizes in the existing cache entry. I think in this way, you >> don't need the cache to be transactional neither to force the lock on >> reads. Also, I would suggest the copy() to be invoked in your case (or >> in the conditional commands accesses to DataContainer?). > > ^ This is an interesting idea too. While transactions are necessary to combat > ISPN-2956, fixing this via transactions, as you all pointed out, might be a > bit too much and also might hurt performance wise. > > I’m doing further testing to see if I see Takayoshi’s problems locally. In > the mean time, I’ll try to prototype an alternative solution based around > this. > >> >>>> >>>>> >>>>>> This solves the issue explained above, but of course it has an impact of >>>>>> the performance. The test now runs about 1.5 or 2 times slower. >>>>>> >>>>>> This is probably the best that we can do in the 7.0 time frame, but >>>>>> there’s several things that could improve this: >>>>>> >>>>>> 1. True immutable entries in the cache. If the entries in the cache were >>>>>> truly immutable, there would be no risk of sending back a partially >>>>>> correct entry back to the client. >>>>>> >>>>>> 2. A cache replace method that does not compare objects based on >>>>>> equality (the current replace()), but a replace method that takes a >>>>>> function. The function could compare that the old entry’s version and >>>>>> the cached entry’s version match. The function would only be executed >>>>>> inside the inner container, with all the locks held properly. I already >>>>>> hinted something similar in [5]. >>> >>> >>> Shall we raise a new JIRA for the permanent solution then? >> >> +1 >> >> I think the immutable entries would be better, but it will destroy >> ReadCommitted isolation. The ReadCommitted is based on the mutability of >> the entry (i.e. it will not invoke the data container if the entry >> exists in transaction context). When (and if) is removed, then we can >> make the entries immutable. > > Yeah, I think RC is the biggest barrier right now. > > Thanks everyone for their input so far!! :D > > Cheers, > -- > Galder Zamarreño > [email protected] > twitter.com/galderz > > > _______________________________________________ > infinispan-dev mailing list > [email protected] > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño [email protected] twitter.com/galderz _______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
