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
