On 20 Jul 2012, at 16:21, Galder Zamarreño wrote: > Hi, > > Re: https://issues.jboss.org/browse/ISPN-2164 and https://github.com/infinispan/infinispan/pull/1221 > > It could be argued that a remove, regardless of whether it's conditional or not, should not need to have the need for a write skew check, unless there's really been a read before. +1.

> > IOW, the test now passes because the commit throws an exception in 4 out of 5 threads. But, the test could/should maybe work in such way that no exception was thrown and 4 out of 5 invocations returned false to the checks made (conditional remove returns false, or normal remove returns null). > It is possible that multiple tx return the same non-null existing value - I've commented more on the JIRA.

> Cheers, > -- > 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 24 Jul 2012, at 09:54, Galder Zamarreño wrote: >>> >>> >>> IOW, the test now passes because the commit throws an exception in 4 out of 5 threads. But, the test could/should maybe work in such way that no exception was thrown and 4 out of 5 invocations returned false to the checks made (conditional remove returns false, or normal remove returns null). >>> >> It is possible that multiple tx return the same non-null existing value - I've commented more on the JIRA. > > Is it possible under concurrent modification circumstances? > > If you have 5 concurrent removals, one should do the job and the others should be no-ops. IOW, 1/5 of the removes should return non-null and the other 4/5 should return null. This is not the case today. Even it is a bit counterintuitive, with optimistic transactions that's possible though. For example: tx1 and tx2 remove the (k,v) entry concurrently: tx1.remove(k); // "v" is returned as that's what tx1 saw at this time tx2.remove(k); // again "v" is returned, as tx1 hasn't committed anything yet (optimistic transactions) Now during the commit time the writes to k are serialised between tx1 and tx2. The only reliable way in which the user can get this behaviour is by forcing the write skew check through a get first (the workaround describe din ISPN-2164). Now thinking about it, the fact that we return the value to the user during the put is equivalently to a read so it might make sense to perform the write skew check for keys that are put into the cache when unsafeReturn is false (the default) and when the Flag.SKIP_REMOTE_LOOKUP is not used. What do you think?

_______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Mircea, one last thing. Why is there a check for local here? https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... That basically means that concurrent cluster wide conditional removes won't work with OL + RR + writeSkew. Is there a reason why you added this local check?

On Jul 25, 2012, at 1:14 PM, Mircea Markus wrote: > > On 24 Jul 2012, at 20:44, Galder Zamarreño wrote: > >> Mircea, one last thing. Why is there a check for local here? >> >> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... >> >> That basically means that concurrent cluster wide conditional removes won't work with OL + RR + writeSkew. >> >> Is there a reason why you added this local check? > > That code was added with ISPN-1941 and only handles write skew (ws) for local caches, Manik might comment a bit more about it. > The code that handles ws for distributed caches is in the VersionedEntryWrappingInterceptor(VEWI). > > I don't think that that code you pointed belongs to the OptimisticLockingInterceptor (OLI) for two reasons: the OLI is should handle the locking and write skew checking is an orthogonal concern. Also the rest of write skew checking is handled in the VEWI, so this logic should be placed there as well. > TBH I think write skew check logic deserve its own dedicated interceptor as the code in this area is spread over too many places: OLI, VEWI and some not nice static methods in the ClusteringDependentLogic. At least for me it is quite hard to follow it as it is now. Wdyt? Good points. So, we need a better way of doing write skew checks both local and in cluster, from a more centralized place then? Let's see what Manik and others say, but won't be able to properly fix this before I go on holidays. There's a valid workaround for it though, so not massively urgent.

I didn't read all the details on the optimistick lock implementation, but want to warn here against the interpretation of the SKIP_REMOTE_LOOKUP flag for these purposes; it should only apply on REMOTE lookups, not imply a general-purpose "I'm ignoring the return value", that would be misleading for users as it's not how things work, it's not automatically implying for example also a skip from cache stores or even a local data container GET hit.. which could affect access statistics and eviction decisions. https://github.com/infinispan/infinispan/pull/1221#r1310549 On 25 July 2012 14:27, Mircea Markus <mircea.markus(a)jboss.com&gt; wrote: > > On 25 Jul 2012, at 12:26, Galder Zamarreño wrote: > > > On Jul 25, 2012, at 1:14 PM, Mircea Markus wrote: > > > On 24 Jul 2012, at 20:44, Galder Zamarreño wrote: > > > Mircea, one last thing. Why is there a check for local here? > > > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... > > > That basically means that concurrent cluster wide conditional removes won't > work with OL + RR + writeSkew. > > > Is there a reason why you added this local check? > > > That code was added with ISPN-1941 and only handles write skew (ws) for > local caches, Manik might comment a bit more about it. > > The code that handles ws for distributed caches is in the > VersionedEntryWrappingInterceptor(VEWI). > > > I don't think that that code you pointed belongs to the > OptimisticLockingInterceptor (OLI) for two reasons: the OLI is should handle > the locking and write skew checking is an orthogonal concern. Also the rest > of write skew checking is handled in the VEWI, so this logic should be > placed there as well. > > TBH I think write skew check logic deserve its own dedicated interceptor as > the code in this area is spread over too many places: OLI, VEWI and some > not nice static methods in the ClusteringDependentLogic. At least for me it > is quite hard to follow it as it is now. Wdyt? > > > Good points. So, we need a better way of doing write skew checks both local > and in cluster, from a more centralized place then? > > Let's see what Manik and others say, but won't be able to properly fix this > before I go on holidays. There's a valid workaround for it though, so not > massively urgent. > > +1. > > > _______________________________________________ > 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

Good explanation, I totally agree. Using SKIP_REMOTE_LOOKUP will always include some degree of inconsistent behaviour depending on the node it is run on, and as such is obviously reserved for very special use cases and only for users who understand the implications. We discussed a year ago about introducing an API safe version of IGNORE_RETURN_VALUES, which resulted in the Jokre project https://github.com/infinispan/jokre This isn't much active anymore as I was told that the JSR will have put() and other write operations having an explicit void return type. Anyway, +1 from me to introduce a IGNORE_RETURN_VALUES flag to be used today and as internal building block for these operations in the future. On 6 August 2012 14:09, Mircea Markus <mircea.markus(a)jboss.com&gt; wrote: > Very good point. > I rdeplied inline: > https://github.com/infinispan/infinispan/pull/1221#discussion_r1310710 > > On 6 Aug 2012, at 11:27, Sanne Grinovero wrote: > > I didn't read all the details on the optimistick lock implementation, > but want to warn here against the interpretation of the > SKIP_REMOTE_LOOKUP flag for these purposes; it should only apply on > REMOTE lookups, not imply a general-purpose "I'm ignoring the return > value", that would be misleading for users as it's not how things > work, it's not automatically implying for example also a skip from > cache stores or even a local data container GET hit.. which could > affect access statistics and eviction decisions. > > https://github.com/infinispan/infinispan/pull/1221#r1310549 > > On 25 July 2012 14:27, Mircea Markus <mircea.markus(a)jboss.com&gt; wrote: > > > On 25 Jul 2012, at 12:26, Galder Zamarreño wrote: > > > > On Jul 25, 2012, at 1:14 PM, Mircea Markus wrote: > > > > On 24 Jul 2012, at 20:44, Galder Zamarreño wrote: > > > > Mircea, one last thing. Why is there a check for local here? > > > > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... > > > > That basically means that concurrent cluster wide conditional removes won't > > work with OL + RR + writeSkew. > > > > Is there a reason why you added this local check? > > > > That code was added with ISPN-1941 and only handles write skew (ws) for > > local caches, Manik might comment a bit more about it. > > > The code that handles ws for distributed caches is in the > > VersionedEntryWrappingInterceptor(VEWI). > > > > I don't think that that code you pointed belongs to the > > OptimisticLockingInterceptor (OLI) for two reasons: the OLI is should handle > > the locking and write skew checking is an orthogonal concern. Also the rest > > of write skew checking is handled in the VEWI, so this logic should be > > placed there as well. > > > TBH I think write skew check logic deserve its own dedicated interceptor as > > the code in this area is spread over too many places: OLI, VEWI and some > > not nice static methods in the ClusteringDependentLogic. At least for me it > > is quite hard to follow it as it is now. Wdyt? > > > > Good points. So, we need a better way of doing write skew checks both local > > and in cluster, from a more centralized place then? > > > Let's see what Manik and others say, but won't be able to properly fix this > > before I go on holidays. There's a valid workaround for it though, so not > > massively urgent. > > > +1. > > > > _______________________________________________ > > 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 > > > > _______________________________________________ > 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