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: >> >>> Hi all, >>> >>> Last few days I’ve been having fun fixing [1] and the solution I’ve got to >>> has some links to [2]. >>> >>> The problem is that when getCacheEntry() is called, the entry returned >>> could be partially correct. Up until now, we’d send back a cache entry back >>> to the client that the user could not modify, but it was linked to the data >>> container cache entry, which is mutable and hence could change on the fly. >>> As a result of this, the value could sometimes be updated but the version >>> would belong to a previous value for example. >>> >>> A first attempt to fix it was to provide a true immutable cache entry view >>> which was initialised on construction, but even here there was the chance >>> of having value and version mistmatch, because getCacheEntry does not >>> acquire locks, so an ongoing update could result in the cache entry being >>> constructed when the update was half way, so, it would have the right value >>> but the old version information. >>> >>> All this didn’t work well with the replaceIfUmodified logic in [3]. For >>> example, you could easily get this situation: >>> >>> Current Cache contents: key=k1, value=A, version=1 >>> >>> T1: Hot Rod client calls: replace(k1, value=B, old-version=1) >>> T2: Hot Rod client calls: replace(k1, value=B, old-version=1) >>> T1: Server calls getCacheEntry and retrieves value=A,version=1 >>> T1: Cached and stream versions match (both are 1), so call replace(k1, >>> old-value=A, new-value=B) >>> T1: Server updates value to B but version is still 1 >>> T2: Server calls getCacheEntry and retrieves value=B,version=1 (wrong!) >>> T1: Cached and stream versions match (both are 1), so call replace(k1, >>> old-value=B, new-value=B) >>> T1: Server updates version to 2 >>> T1: Returns Success, replace worked >>> T2: Returns Success, replace worked >>> >>> The end result is that cache contained B, but both replaces returned true. >>> This is wrong and would fail to consistenly keep a counter in the value >>> part. Imagine the value being a counter of number of times the replace >>> succeeded. In the test developed by Takayoshi, N times replace() would >>> return true, but the final value would be N-1 or N-2, or N-5 :| >>> >>> 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. > >> >>> 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? >>> >>> Finally, this was not a problem when the value stored in Hot Rod was a >>> ByteArrayValue wrapping the byte array and the version, because the value >>> was treated atomically, and in hindsight, maybe adding getCacheEntry might >>> have been premature, but this method has proven useful for other use cases >>> too (rolling upgrades…etc). >>> >>> Cheers, >>> >>> [1] https://issues.jboss.org/browse/ISPN-4424 >>> [2] https://issues.jboss.org/browse/ISPN-2956 >>> [3] >>> https://github.com/infinispan/infinispan/blob/master/server/core/src/main/scala/org/infinispan/server/core/AbstractProtocolDecoder.scala#L302 >>> [4] https://github.com/galderz/infinispan/tree/t_4424 >>> [5] >>> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/AdvancedCache.java#L374 >>> -- >>> Galder Zamarreño >>> [email protected] >>> twitter.com/galderz >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> [email protected] >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> Cheers, >> -- >> Mircea Markus >> Infinispan lead (www.infinispan.org) >> >> >> >> >> >> _______________________________________________ >> 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 Cheers, -- Mircea Markus Infinispan lead (www.infinispan.org) _______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
