Comments that were outstanding on PR: @danberindei: > +1 to move the discussion to the mailing list, could you summarize your changes (preferably for both non-tx and tx cases) and send an email to the list? > And now to add some more to this already unwieldy discussion :) > 1. Many interceptors check ownership, I don't think that would be a problem. Besides, I think creating a new L1ReadSynchronizer for every read is just as bad for performance as locking the key on every read, so you'd need that check either way. > We can't use a try lock approach for L1 invalidation when gets are locked without possibly dropping a L1 update We can't use a timed lock approach for L1 invalidation when writes lock the key as we could get into a timed deadlock situation when another node concurrently writes to a value/stripe I still don't see how we can get away with locking on a get. What are you proposing?

> 2. By default L1 invalidations are sent as multicasts, so I'm not sure ISPN-3273 really matters here. BTW, I wonder if we have a check to only send L1 invalidations from one node if the threshold is 0... I agree that is the default, but we should support the operation, although it doesn't matter for this discussion. Also I am curious as to why multicast for L1 isn't set to say 2 by default? It seems wasteful to send a multicast to all members that they process when only 1 would do anything about it. Do you know why this is like that?

> 3a. Right, for put commands we can't hold the local lock while executing the remote put, or we'll have a deadlock. But I think a shorter lock, held only after the remote put completed (or after the lock on the primary owner was acquired, with txs) should work. Same point under 1

> > 3b. We'd also have an ownership check before, so we'd only serialize the get commands that need to go remotely for the same key. I think it would be almost the same as your solution (although it does have one ? disadvantage - if the key doesn't exist in the cache, all the get commands will go remotely). The number of L1 writes should be very small compared to the number of L1 reads anyway, otherwise it would be more efficient to get the key from the owner every time. You are saying an optimization for owner nodes so they don't do the "corralling" for keys they own? I like that. Also I don't think it has the disadvantage, it only does remotes it if isn't an owner.

> > It would be nice to agree on what guarantees we want to provide for L1 invalidation in non-tx caches, I'm not sure if we can do anything to prevent this scenario: Actually this scenario doesn't occur with non-tx since writes don't update the L1 with their value, they just invalidate. Tx caches are fine with this because they acquire the primary owner lock for the duration of the write including the L1 update so you can't have this ordering.

> > A initiates a put(k, v1) to the primary owner B > B performs the put(k, v1), invalidates every non-owner and returns > B performs another put(k, v2), invalidating every non-owner > A receives the result from B and puts k=v1 in its L1 @pruivo: > The invalidation does not need to wait for the remote get. When you receive an invalidation, you can mark the current remote get invalid. The invalidation command can return immediately and the remote get can be repeated. Also, it removes the key from data container (if exists) Dan hit it right in the head. Unfortunately there is no guarantee the cancellation can work properly, so it is a best effort and if not wait until we know we will invalidate properly. > The writes can update the L1 through your L1Synchronized by adding a simple method like updateL1(newValue). The blocking threads will return immediately the new value and they don't need to wait for the reply. Non tx cache write operations aren't safe to update L1 with the value since they don't acquire the owning lock while updating the L1, which means you could have interleaved writes. Which is the primary reason I rejected ISPN- 3214. For tx caches we can't do this since the update has to take part of the tx, which the get would be updating the L1 outside of a transaction. > I see... However, I think that all the events should synchronize at some point (update by remote get, update by local put and invalidation). I was hoping that would cover this. Other than the outstanding issue in ISPN-2965. On Thu, Jun 27, 2013 at 9:18 AM, William Burns <mudokonman(a)gmail.com&gt; wrote: > First off I apologize for the length. > > There have been a few Jiras recently that have identified L1 consistency > issues with both TX and non TX sync caches. Async caches with L1 have their > own issues as well, but I only wanted to talk about sync caches. > > https://issues.jboss.org/browse/ISPN-3197 > https://issues.jboss.org/browse/ISPN-2965 > https://issues.jboss.org/browse/ISPN-2990 > > I have proposed a solution in > https://github.com/infinispan/infinispan/pull/1922 which should start L1 > consistency down the right track. There are quite a few comments on it if > you want to look into it more, but because of that I am moving this to the > dev mailing list. > > The key changes in the PR are the following (non-tx): > > 1. Concurrent reads for a key that can retrieve a remote value are > "corralled" into a single thread of execution for that given key. This > would reduce network traffic with concurrent gets for the same key. Note > the "corralling" only happens on a per key basis. > 2. The single thread that is doing the remote get would update the L1 if > able (without locking) and make available the value to all the requests > waiting on the get. > 3. Invalidations that are received would first check to see if there is a > current remote get occurring for it's keys. If there is it will attempt to > cancel the L1 write(s) before it occurs. If it cannot cancel the L1 write, > then it must also wait on the current remote get completion and subsequently > run the invalidation. Note the cancellation would fail when the remote get > was done and it is in the middle of updating the L1, so this would be very > small window. > 4. Local writes will also do the same thing as the invalidation with > cancelling or waiting. Note that non tx local writes only do L1 > invalidations and don't write the value to the data container. Reasons why > I found at https://issues.jboss.org/browse/ISPN-3214 > 5. Writes that require the previous value and don't have it in the L1 would > also do it's get operations using the same "corralling" method. > > 4/5 are not currently implemented in PR. > > This approach would use no locking for non tx caches for all L1 operations. > The synchronization point would be done through the "corralling" method and > invalidations/writes communicating to it. > > Transactional caches would do almost the same thing as non-tx. Note these > changes are not done in any way yet. > > 1. Gets would now update the L1 immediately after retrieving the value > without locking, but still using the "corralling" technique that non-tx > does. Previously the L1 update from a get was transactional. This actually > would remedy issue [1] > 2. Writes currently acquire the remote lock when committing, which is why tx > caches are able to update the L1 with the value. Writes would do the same > cancellation/wait method as non-tx. > 3. Writes that require the previous value and don't have it in the L1 would > also do it's get operations using the same method. > 4. For tx cache [2] would also have to be done. > > [1] - > https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... > [2] - https://issues.jboss.org/browse/ISPN-1540 > > Also rehashing is another issue, but we should be able to acquire the state > transfer lock before updating the L1 on a get, just like when an entry is > committed to the data container. > > Any comments/concerns would be appreciated. > > Thanks, > > - Will _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Thu, Jun 27, 2013 at 4:40 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > On Thu, Jun 27, 2013 at 4:40 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: >> >> Comments that were outstanding on PR: >> >> @danberindei: >> >> > +1 to move the discussion to the mailing list, could you summarize >> your changes (preferably for both non-tx and tx cases) and send an >> email to the list? >> > And now to add some more to this already unwieldy discussion :) >> >> > 1. Many interceptors check ownership, I don't think that would be >> a problem. Besides, I think creating a new L1ReadSynchronizer for >> every read is just as bad for performance as locking the key on every >> read, so you'd need that check either way. >> > >> We can't use a try lock approach for L1 invalidation when gets are >> locked without possibly dropping a L1 update >> We can't use a timed lock approach for L1 invalidation when writes >> lock the key as we could get into a timed deadlock situation when >> another node concurrently writes to a value/stripe >> >> I still don't see how we can get away with locking on a get. What are >> you proposing? > > > I'm proposing something like this: > > 1. For get commands: acquire local lock + remote get + store in L1 + release > local lock > 2. For invalidate commands: acquire local lock + remove entry from L1 + > release local lock > 3. For write commands: invoke on primary owner, and the primary owner sends > invalidation back to originator; alternatively, skip the invalidation > command to the originator and do invoke on primary owner + acquire local > lock + remove entry from L1 + release local lock I do like this and think it would be simpler for non-tx. However this will still not be as performant I wouldn't think (just a gut feeling).

occur, which is more likely with lock striping.

I don't think this however is feasible for tx caches. Optimistic - we don't want to have gets being blocked when the tx is being committed - that could be quite a bit of a performance hit especially if it has to do a 2 phase commit. Updating the L1 on a remoteGetBeforeWrites could help remedy this (since get wouldn't need to lock), however then you would be almost implementing what I have and still have locking overhead. Maybe this would occur so infrequently it wouldn't matter though?

Pessimistic - I haven't looked into pessimistic closely yet, but in that case I don't think it ever acquires the local locks so it would have to acquire a remote lock on a get, which would be pretty disastrous (gets would indirectly have a weak FORCE_WRITE_LOCK enabled). Making this to acquire local locks might work as well, but then would still have the same issue as optimistic when committing - I really am not as familiar with Pessimistic to say for sure.

> > All the lock acquisitions would use a regular timeout, not a try lock > approach. In this case they could, yes. > >> >> > 2. By default L1 invalidations are sent as multicasts, so I'm not sure >> > ISPN-3273 really matters here. BTW, I wonder if we have a check to only send >> > L1 invalidations from one node if the threshold is 0... >> I agree that is the default, but we should support the operation, >> although it doesn't matter for this discussion. Also I am curious as >> to why multicast for L1 isn't set to say 2 by default? It seems >> wasteful to send a multicast to all members that they process when >> only 1 would do anything about it. Do you know why this is like that? > > > I suppose it's because we don't have good perf numbers for different L1 > invalidation threshold numbers... > > The problem is, we don't have a way to count all the requestors of a key in > the cluster, so it's reasonably likely that with a threshold of 2 you'd get > 1 unicast invalidation from one owner + 1 multicast invalidation from the > other owner, making it less efficient than a single multicast invalidation. It is a nitpick anyways, and really shouldn't make that big of a difference. >> > >> > 3a. Right, for put commands we can't hold the local lock while >> > executing the remote put, or we'll have a deadlock. But I think a shorter >> > lock, held only after the remote put completed (or after the lock on the >> > primary owner was acquired, with txs) should work. >> Same point under 1 > > > I don't see how we could get a deadlock if we don't hold the local lock > during the remote write invocation. Agree. > >> >> > >> > 3b. We'd also have an ownership check before, so we'd only serialize >> > the get commands that need to go remotely for the same key. I think it would >> > be almost the same as your solution (although it does have one ? >> > disadvantage - if the key doesn't exist in the cache, all the get commands >> > will go remotely). The number of L1 writes should be very small compared to >> > the number of L1 reads anyway, otherwise it would be more efficient to get >> > the key from the owner every time. >> You are saying an optimization for owner nodes so they don't do the >> "corralling" for keys they own? I like that. Also I don't think it >> has the disadvantage, it only does remotes it if isn't an owner. > > > I meant your corralling strategy means if you have 2 concurrent get commands > and one of them retrieves a null from the entry owners, the other command > will return null directly. With regular locking, the other command wouldn't > find anything in L1 and it would do another remote get. > > I don't think there's any disadvantage in skipping the corralling for key > owners, in fact I think we need to skip it if the key already exists in L1, > too. +1 > >> >> > >> > It would be nice to agree on what guarantees we want to provide for L1 >> > invalidation in non-tx caches, I'm not sure if we can do anything to prevent >> > this scenario: >> Actually this scenario doesn't occur with non-tx since writes don't >> update the L1 with their value, they just invalidate. Tx caches are >> fine with this because they acquire the primary owner lock for the >> duration of the write including the L1 update so you can't have this >> ordering. > > > Sounds good. > >> >> > >> > A initiates a put(k, v1) to the primary owner B >> > B performs the put(k, v1), invalidates every non-owner and returns >> > B performs another put(k, v2), invalidating every non-owner >> > A receives the result from B and puts k=v1 in its L1 >> >> @pruivo: >> >> > The invalidation does not need to wait for the remote get. When you >> > receive an invalidation, you can mark the current remote get invalid. The >> > invalidation command can return immediately and the remote get can be >> > repeated. Also, it removes the key from data container (if exists) >> Dan hit it right in the head. Unfortunately there is no guarantee the >> cancellation can work properly, so it is a best effort and if not wait >> until we know we will invalidate properly. >> > The writes can update the L1 through your L1Synchronized by adding a >> > simple method like updateL1(newValue). The blocking threads will return >> > immediately the new value and they don't need to wait for the reply. >> Non tx cache write operations aren't safe to update L1 with the value >> since they don't acquire the owning lock while updating the L1, which >> means you could have interleaved writes. Which is the primary reason >> I rejected ISPN- 3214. For tx caches we can't do this since the >> update has to take part of the tx, which the get would be updating the >> L1 outside of a transaction. >> > I see... However, I think that all the events should synchronize at some >> > point (update by remote get, update by local put and invalidation). >> I was hoping that would cover this. Other than the outstanding issue >> in ISPN-2965. >> >> On Thu, Jun 27, 2013 at 9:18 AM, William Burns <mudokonman(a)gmail.com&gt; >> wrote: >> > First off I apologize for the length. >> > >> > There have been a few Jiras recently that have identified L1 consistency >> > issues with both TX and non TX sync caches. Async caches with L1 have >> > their >> > own issues as well, but I only wanted to talk about sync caches. >> > >> > https://issues.jboss.org/browse/ISPN-3197 >> > https://issues.jboss.org/browse/ISPN-2965 >> > https://issues.jboss.org/browse/ISPN-2990 >> > >> > I have proposed a solution in >> > https://github.com/infinispan/infinispan/pull/1922 which should start L1 >> > consistency down the right track. There are quite a few comments on it >> > if >> > you want to look into it more, but because of that I am moving this to >> > the >> > dev mailing list. >> > >> > The key changes in the PR are the following (non-tx): >> > >> > 1. Concurrent reads for a key that can retrieve a remote value are >> > "corralled" into a single thread of execution for that given key. This >> > would reduce network traffic with concurrent gets for the same key. >> > Note >> > the "corralling" only happens on a per key basis. >> > 2. The single thread that is doing the remote get would update the L1 if >> > able (without locking) and make available the value to all the requests >> > waiting on the get. >> > 3. Invalidations that are received would first check to see if there is >> > a >> > current remote get occurring for it's keys. If there is it will attempt >> > to >> > cancel the L1 write(s) before it occurs. If it cannot cancel the L1 >> > write, >> > then it must also wait on the current remote get completion and >> > subsequently >> > run the invalidation. Note the cancellation would fail when the remote >> > get >> > was done and it is in the middle of updating the L1, so this would be >> > very >> > small window. >> > 4. Local writes will also do the same thing as the invalidation with >> > cancelling or waiting. Note that non tx local writes only do L1 >> > invalidations and don't write the value to the data container. Reasons >> > why >> > I found at https://issues.jboss.org/browse/ISPN-3214 >> > 5. Writes that require the previous value and don't have it in the L1 >> > would >> > also do it's get operations using the same "corralling" method. >> > >> > 4/5 are not currently implemented in PR. >> > >> > This approach would use no locking for non tx caches for all L1 >> > operations. >> > The synchronization point would be done through the "corralling" method >> > and >> > invalidations/writes communicating to it. >> > >> > Transactional caches would do almost the same thing as non-tx. Note >> > these >> > changes are not done in any way yet. >> > >> > 1. Gets would now update the L1 immediately after retrieving the value >> > without locking, but still using the "corralling" technique that non-tx >> > does. Previously the L1 update from a get was transactional. This >> > actually >> > would remedy issue [1] >> > 2. Writes currently acquire the remote lock when committing, which is >> > why tx >> > caches are able to update the L1 with the value. Writes would do the >> > same >> > cancellation/wait method as non-tx. >> > 3. Writes that require the previous value and don't have it in the L1 >> > would >> > also do it's get operations using the same method. >> > 4. For tx cache [2] would also have to be done. >> > >> > [1] - >> > >> > https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... >> > [2] - https://issues.jboss.org/browse/ISPN-1540 >> > >> > Also rehashing is another issue, but we should be able to acquire the >> > state >> > transfer lock before updating the L1 on a get, just like when an entry >> > is >> > committed to the data container. >> > >> > Any comments/concerns would be appreciated. >> > >> > Thanks, >> > >> > - Will >> >> _______________________________________________ >> 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

On Fri, Jun 28, 2013 at 5:53 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > On Fri, Jun 28, 2013 at 12:51 AM, William Burns <mudokonman(a)gmail.com&gt; > wrote: >> >> On Thu, Jun 27, 2013 at 4:40 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; >> wrote: >> > >> > >> > On Thu, Jun 27, 2013 at 4:40 PM, William Burns <mudokonman(a)gmail.com&gt; >> > wrote: >> >> >> >> Comments that were outstanding on PR: >> >> >> >> @danberindei: >> >> >> >> > +1 to move the discussion to the mailing list, could you summarize >> >> your changes (preferably for both non-tx and tx cases) and send an >> >> email to the list? >> >> > And now to add some more to this already unwieldy discussion :) >> >> >> >> > 1. Many interceptors check ownership, I don't think that would be >> >> a problem. Besides, I think creating a new L1ReadSynchronizer for >> >> every read is just as bad for performance as locking the key on every >> >> read, so you'd need that check either way. >> >> > >> >> We can't use a try lock approach for L1 invalidation when gets are >> >> locked without possibly dropping a L1 update >> >> We can't use a timed lock approach for L1 invalidation when writes >> >> lock the key as we could get into a timed deadlock situation when >> >> another node concurrently writes to a value/stripe >> >> >> >> I still don't see how we can get away with locking on a get. What are >> >> you proposing? >> > >> > >> > I'm proposing something like this: >> > >> > 1. For get commands: acquire local lock + remote get + store in L1 + >> > release >> > local lock >> > 2. For invalidate commands: acquire local lock + remove entry from L1 + >> > release local lock >> > 3. For write commands: invoke on primary owner, and the primary owner >> > sends >> > invalidation back to originator; alternatively, skip the invalidation >> > command to the originator and do invoke on primary owner + acquire local >> > lock + remove entry from L1 + release local lock >> >> I do like this and think it would be simpler for non-tx. However this >> will still not be as performant I wouldn't think (just a gut feeling). > > > It would indeed be less performant in some cases, e.g. if we have two get > commands for a remote key at the same time and the key doesn't exist, my > proposal would request the key twice. > > The writes to L1 *should* be rare enough that the extra lock contention > there would be minimal. There is a chance that an invalidation will be stuck > waiting for a remote get, so it will again be slower than your approach. > >> However I am more than certain it will be slower when collisions >> occur, which is more likely with lock striping. >> > > I'm not that worried about lock striping - the user can always increase the > number of stripes to get less contention. And I hope they know better than > to use lock striping and transactions with multiple keys, or they'll get > deadlocks with or without L1 enabled. I didn't think of the deadlock problem with tx and lock striping. I am wondering should we log a warning message so that user's get an explicit warning?

>> >> I don't think this however is feasible for tx caches. >> >> Optimistic - we don't want to have gets being blocked when the tx is >> being committed - that could be quite a bit of a performance hit >> especially if it has to do a 2 phase commit. Updating the L1 on a >> remoteGetBeforeWrites could help remedy this (since get wouldn't need >> to lock), however then you would be almost implementing what I have >> and still have locking overhead. Maybe this would occur so >> infrequently it wouldn't matter though? >> > > Yes, I would propose removing the L1 commit in EntryWrappingInterceptor as > well, and moving it to the L1 interceptor - immediately after the value was > retrieved from the remote owner. The local lock would also be released > immediately. > > The part I wanted to avoid from your proposal was > L1ReadSync/L1ReadSynchronizer, because I found it kind of hard to > understand. I guess I'd find it more approachable all the synchronization > (the map of L1ReadSynchronizers and the interaction with it) would be in a > separate class, with its own tests, and the L1 interceptor would only know > about the high-level stuff. Like the LockManager also hides a map of locks; > not like the LockManager unit tests, because they're kind of lacking, but > LockManager has the advantage that almost all the tests touch it indirectly. > True, I agree I like it as a separate class better - much better separation. Maybe I can move that over and add tests and see if that makes it more clear? If not we can still always use the locking approach.

>> >> Pessimistic - I haven't looked into pessimistic closely yet, but in >> that case I don't think it ever acquires the local locks so it would >> have to acquire a remote lock on a get, which would be pretty >> disastrous (gets would indirectly have a weak FORCE_WRITE_LOCK >> enabled). Making this to acquire local locks might work as well, but >> then would still have the same issue as optimistic when committing - I >> really am not as familiar with Pessimistic to say for sure. >> > > Nope, acquiring a local lock should be enough. We don't want to prevent > other txs from modifying the key at all, we just want to delay the > invalidation commands triggered by those modifications on the local node. > And the lock would only be held for the duration of the remote get + L1 > update, not all the way to the commit. > Okay, cool. I didn't know for sure. >> >> > >> > All the lock acquisitions would use a regular timeout, not a try lock >> > approach. >> In this case they could, yes. >> > >> >> >> >> > 2. By default L1 invalidations are sent as multicasts, so I'm not >> >> > sure >> >> > ISPN-3273 really matters here. BTW, I wonder if we have a check to >> >> > only send >> >> > L1 invalidations from one node if the threshold is 0... >> >> I agree that is the default, but we should support the operation, >> >> although it doesn't matter for this discussion. Also I am curious as >> >> to why multicast for L1 isn't set to say 2 by default? It seems >> >> wasteful to send a multicast to all members that they process when >> >> only 1 would do anything about it. Do you know why this is like that? >> > >> > >> > I suppose it's because we don't have good perf numbers for different L1 >> > invalidation threshold numbers... >> > >> > The problem is, we don't have a way to count all the requestors of a key >> > in >> > the cluster, so it's reasonably likely that with a threshold of 2 you'd >> > get >> > 1 unicast invalidation from one owner + 1 multicast invalidation from >> > the >> > other owner, making it less efficient than a single multicast >> > invalidation. >> >> It is a nitpick anyways, and really shouldn't make that big of a >> difference. >> >> >> > >> >> > >> >> > 3a. Right, for put commands we can't hold the local lock while >> >> > executing the remote put, or we'll have a deadlock. But I think a >> >> > shorter >> >> > lock, held only after the remote put completed (or after the lock on >> >> > the >> >> > primary owner was acquired, with txs) should work. >> >> Same point under 1 >> > >> > >> > I don't see how we could get a deadlock if we don't hold the local lock >> > during the remote write invocation. >> >> Agree. >> >> > >> >> >> >> > >> >> > 3b. We'd also have an ownership check before, so we'd only serialize >> >> > the get commands that need to go remotely for the same key. I think >> >> > it would >> >> > be almost the same as your solution (although it does have one ? >> >> > disadvantage - if the key doesn't exist in the cache, all the get >> >> > commands >> >> > will go remotely). The number of L1 writes should be very small >> >> > compared to >> >> > the number of L1 reads anyway, otherwise it would be more efficient >> >> > to get >> >> > the key from the owner every time. >> >> You are saying an optimization for owner nodes so they don't do the >> >> "corralling" for keys they own? I like that. Also I don't think it >> >> has the disadvantage, it only does remotes it if isn't an owner. >> > >> > >> > I meant your corralling strategy means if you have 2 concurrent get >> > commands >> > and one of them retrieves a null from the entry owners, the other >> > command >> > will return null directly. With regular locking, the other command >> > wouldn't >> > find anything in L1 and it would do another remote get. >> > >> > I don't think there's any disadvantage in skipping the corralling for >> > key >> > owners, in fact I think we need to skip it if the key already exists in >> > L1, >> > too. >> +1 >> > >> >> >> >> > >> >> > It would be nice to agree on what guarantees we want to provide for >> >> > L1 >> >> > invalidation in non-tx caches, I'm not sure if we can do anything to >> >> > prevent >> >> > this scenario: >> >> Actually this scenario doesn't occur with non-tx since writes don't >> >> update the L1 with their value, they just invalidate. Tx caches are >> >> fine with this because they acquire the primary owner lock for the >> >> duration of the write including the L1 update so you can't have this >> >> ordering. >> > >> > >> > Sounds good. >> > >> >> >> >> > >> >> > A initiates a put(k, v1) to the primary owner B >> >> > B performs the put(k, v1), invalidates every non-owner and returns >> >> > B performs another put(k, v2), invalidating every non-owner >> >> > A receives the result from B and puts k=v1 in its L1 >> >> >> >> @pruivo: >> >> >> >> > The invalidation does not need to wait for the remote get. When you >> >> > receive an invalidation, you can mark the current remote get invalid. >> >> > The >> >> > invalidation command can return immediately and the remote get can be >> >> > repeated. Also, it removes the key from data container (if exists) >> >> Dan hit it right in the head. Unfortunately there is no guarantee the >> >> cancellation can work properly, so it is a best effort and if not wait >> >> until we know we will invalidate properly. >> >> > The writes can update the L1 through your L1Synchronized by adding a >> >> > simple method like updateL1(newValue). The blocking threads will >> >> > return >> >> > immediately the new value and they don't need to wait for the reply. >> >> Non tx cache write operations aren't safe to update L1 with the value >> >> since they don't acquire the owning lock while updating the L1, which >> >> means you could have interleaved writes. Which is the primary reason >> >> I rejected ISPN- 3214. For tx caches we can't do this since the >> >> update has to take part of the tx, which the get would be updating the >> >> L1 outside of a transaction. >> >> > I see... However, I think that all the events should synchronize at >> >> > some >> >> > point (update by remote get, update by local put and invalidation). >> >> I was hoping that would cover this. Other than the outstanding issue >> >> in ISPN-2965. >> >> >> >> On Thu, Jun 27, 2013 at 9:18 AM, William Burns <mudokonman(a)gmail.com > >> >> wrote: >> >> > First off I apologize for the length. >> >> > >> >> > There have been a few Jiras recently that have identified L1 >> >> > consistency >> >> > issues with both TX and non TX sync caches. Async caches with L1 >> >> > have >> >> > their >> >> > own issues as well, but I only wanted to talk about sync caches. >> >> > >> >> > https://issues.jboss.org/browse/ISPN-3197 >> >> > https://issues.jboss.org/browse/ISPN-2965 >> >> > https://issues.jboss.org/browse/ISPN-2990 >> >> > >> >> > I have proposed a solution in >> >> > https://github.com/infinispan/infinispan/pull/1922 which should start >> >> > L1 >> >> > consistency down the right track. There are quite a few comments on >> >> > it >> >> > if >> >> > you want to look into it more, but because of that I am moving this >> >> > to >> >> > the >> >> > dev mailing list. >> >> > >> >> > The key changes in the PR are the following (non-tx): >> >> > >> >> > 1. Concurrent reads for a key that can retrieve a remote value are >> >> > "corralled" into a single thread of execution for that given key. >> >> > This >> >> > would reduce network traffic with concurrent gets for the same key. >> >> > Note >> >> > the "corralling" only happens on a per key basis. >> >> > 2. The single thread that is doing the remote get would update the L1 >> >> > if >> >> > able (without locking) and make available the value to all the >> >> > requests >> >> > waiting on the get. >> >> > 3. Invalidations that are received would first check to see if there >> >> > is >> >> > a >> >> > current remote get occurring for it's keys. If there is it will >> >> > attempt >> >> > to >> >> > cancel the L1 write(s) before it occurs. If it cannot cancel the L1 >> >> > write, >> >> > then it must also wait on the current remote get completion and >> >> > subsequently >> >> > run the invalidation. Note the cancellation would fail when the >> >> > remote >> >> > get >> >> > was done and it is in the middle of updating the L1, so this would be >> >> > very >> >> > small window. >> >> > 4. Local writes will also do the same thing as the invalidation with >> >> > cancelling or waiting. Note that non tx local writes only do L1 >> >> > invalidations and don't write the value to the data container. >> >> > Reasons >> >> > why >> >> > I found at https://issues.jboss.org/browse/ISPN-3214 >> >> > 5. Writes that require the previous value and don't have it in the L1 >> >> > would >> >> > also do it's get operations using the same "corralling" method. >> >> > >> >> > 4/5 are not currently implemented in PR. >> >> > >> >> > This approach would use no locking for non tx caches for all L1 >> >> > operations. >> >> > The synchronization point would be done through the "corralling" >> >> > method >> >> > and >> >> > invalidations/writes communicating to it. >> >> > >> >> > Transactional caches would do almost the same thing as non-tx. Note >> >> > these >> >> > changes are not done in any way yet. >> >> > >> >> > 1. Gets would now update the L1 immediately after retrieving the >> >> > value >> >> > without locking, but still using the "corralling" technique that >> >> > non-tx >> >> > does. Previously the L1 update from a get was transactional. This >> >> > actually >> >> > would remedy issue [1] >> >> > 2. Writes currently acquire the remote lock when committing, which is >> >> > why tx >> >> > caches are able to update the L1 with the value. Writes would do the >> >> > same >> >> > cancellation/wait method as non-tx. >> >> > 3. Writes that require the previous value and don't have it in the L1 >> >> > would >> >> > also do it's get operations using the same method. >> >> > 4. For tx cache [2] would also have to be done. >> >> > >> >> > [1] - >> >> > >> >> > >> >> > https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... >> >> > [2] - https://issues.jboss.org/browse/ISPN-1540 >> >> > >> >> > Also rehashing is another issue, but we should be able to acquire the >> >> > state >> >> > transfer lock before updating the L1 on a get, just like when an >> >> > entry >> >> > is >> >> > committed to the data container. >> >> > >> >> > Any comments/concerns would be appreciated. >> >> > >> >> > Thanks, >> >> > >> >> > - Will >> >> >> >> _______________________________________________ >> >> 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Thu, Jun 27, 2013 at 4:18 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: > > First off I apologize for the length. > > There have been a few Jiras recently that have identified L1 consistency > issues with both TX and non TX sync caches. Async caches with L1 have their > own issues as well, but I only wanted to talk about sync caches. > > https://issues.jboss.org/browse/ISPN-3197 > https://issues.jboss.org/browse/ISPN-2965 > https://issues.jboss.org/browse/ISPN-2990 > > I have proposed a solution in > https://github.com/infinispan/infinispan/pull/1922 which should start L1 > consistency down the right track. There are quite a few comments on it if > you want to look into it more, but because of that I am moving this to the > dev mailing list. > > The key changes in the PR are the following (non-tx): > > 1. Concurrent reads for a key that can retrieve a remote value are > "corralled" into a single thread of execution for that given key. This > would reduce network traffic with concurrent gets for the same key. Note > the "corralling" only happens on a per key basis. Get commands on owners should not be serialized. Get commands on non-owners should not be serialized either, if the key already exists in L1. So I'd say L1ReadSynchronizer should be L1WriteSynchronizer instead :)

> > 2. The single thread that is doing the remote get would update the L1 if > able (without locking) and make available the value to all the requests > waiting on the get. Well, L1ReadSynchronizer does prevent other threads from modifying the same key, so we are locking the key - just not using LockManager. It would also require StateTransferLock.acquireSharedTopologyLock() to make sure it doesn't write an L1 entry after the node became a proper owner.

> > 3. Invalidations that are received would first check to see if there is a > current remote get occurring for it's keys. If there is it will attempt to > cancel the L1 write(s) before it occurs. If it cannot cancel the L1 write, > then it must also wait on the current remote get completion and subsequently > run the invalidation. Note the cancellation would fail when the remote get > was done and it is in the middle of updating the L1, so this would be very > small window. I think it would be clearer to describe this as the L1 invalidation cancelling the remote get, not the L1 update, because the actual L1 update can't be cancelled.

We also have to remove the logic in AbstractLockingInterceptor that skips L1 invalidation for a key if it can't acquire a lock with a 0 timeout. > > 4. Local writes will also do the same thing as the invalidation with > cancelling or waiting. Note that non tx local writes only do L1 > invalidations and don't write the value to the data container. Reasons why > I found at https://issues.jboss.org/browse/ISPN-3214 I didn't know about ISPN-3214 or that non-tx writes don't write to L1, but it sounds fair.

> > 5. Writes that require the previous value and don't have it in the L1 > would also do it's get operations using the same "corralling" method. The remoteGetBeforeWrites are a bit different - they don't happen on non-owners, they only happen on writeCH-owners that didn't receive that entry via state transfer yet. They put the value in the InvocationContext, but they don't write it to the data container - nor do they invalidate the L1 entry, if it exists.

> > > 4/5 are not currently implemented in PR. > > This approach would use no locking for non tx caches for all L1 > operations. The synchronization point would be done through the > "corralling" method and invalidations/writes communicating to it. > > Transactional caches would do almost the same thing as non-tx. Note these > changes are not done in any way yet. > > 1. Gets would now update the L1 immediately after retrieving the value > without locking, but still using the "corralling" technique that non-tx > does. Previously the L1 update from a get was transactional. This actually > would remedy issue [1] > > 2. Writes currently acquire the remote lock when committing, which is why > tx caches are able to update the L1 with the value. Writes would do the > same cancellation/wait method as non-tx. > > 3. Writes that require the previous value and don't have it in the L1 > would also do it's get operations using the same method. Just like for non-tx caches, I don't think these remote gets have to be stored in L1.

> > 4. For tx cache [2] would also have to be done. > > [1] - > https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... > [2] - https://issues.jboss.org/browse/ISPN-1540 > > Also rehashing is another issue, but we should be able to acquire the > state transfer lock before updating the L1 on a get, just like when an entry > is committed to the data container. > The same for L1 invalidations - we don't want to remove real entries from the data container after the local node became an owner.

> > Any comments/concerns would be appreciated. > > Thanks, > > - Will > > _______________________________________________ > 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 Fri, Jun 28, 2013 at 12:17 AM, William Burns <mudokonman(a)gmail.com&gt; wrote: > > Trying to leave my points that would most likely have responses to > second email so we can try to get back to a single thread :) > No such luck :) Sorry for sending 2 replies in the first place, but it seemed more natural - I meant to comment on your proposal in one email and to describe my alternative proposal in the second email. > > On Thu, Jun 27, 2013 at 4:12 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; > wrote: > > > > On Thu, Jun 27, 2013 at 4:18 PM, William Burns <mudokonman(a)gmail.com&gt; > > wrote: > >> > >> First off I apologize for the length. > >> > >> There have been a few Jiras recently that have identified L1 > >> consistency > >> issues with both TX and non TX sync caches. Async caches with L1 have > >> their > >> own issues as well, but I only wanted to talk about sync caches. > >> > >> https://issues.jboss.org/browse/ISPN-3197 > >> https://issues.jboss.org/browse/ISPN-2965 > >> https://issues.jboss.org/browse/ISPN-2990 > >> > >> I have proposed a solution in > >> https://github.com/infinispan/infinispan/pull/1922 which should start > >> L1 > >> consistency down the right track. There are quite a few comments on it > >> if > >> you want to look into it more, but because of that I am moving this to > >> the > >> dev mailing list. > >> > >> The key changes in the PR are the following (non-tx): > >> > >> 1. Concurrent reads for a key that can retrieve a remote value are > >> "corralled" into a single thread of execution for that given key. This > >> would reduce network traffic with concurrent gets for the same key. > >> Note > >> the "corralling" only happens on a per key basis. > > > > > > Get commands on owners should not be serialized. Get commands on > > non-owners > > should not be serialized either, if the key already exists in L1. So I'd > > say > > L1ReadSynchronizer should be L1WriteSynchronizer instead :) > > You are suggesting to check the context to see if the key is present > before attempting the synchronizer right? Reading your second email > that seems the case :) > Nope, I meant we should check the data container (aka the L1 cache). But obviously we have to check the invocation context first in a tx cache, if the tx read the key before it should see the same value.

> > > > >> > >> 2. The single thread that is doing the remote get would update the L1 > >> if > >> able (without locking) and make available the value to all the requests > >> waiting on the get. > > > > > > Well, L1ReadSynchronizer does prevent other threads from modifying the > > same > > key, so we are locking the key - just not using LockManager. > > It would also require StateTransferLock.acquireSharedTopologyLock() to > > make > > sure it doesn't write an L1 entry after the node became a proper owner. > > Agree, when I was saying locking I was meaning through the use of the > lock manager. > > > > >> > >> 3. Invalidations that are received would first check to see if there is > >> a > >> current remote get occurring for it's keys. If there is it will > >> attempt to > >> cancel the L1 write(s) before it occurs. If it cannot cancel the L1 > >> write, > >> then it must also wait on the current remote get completion and > >> subsequently > >> run the invalidation. Note the cancellation would fail when the remote > >> get > >> was done and it is in the middle of updating the L1, so this would be > >> very > >> small window. > > > > > > I think it would be clearer to describe this as the L1 invalidation > > cancelling the remote get, not the L1 update, because the actual L1 > > update > > can't be cancelled. > > When I say L1 update I meant the write to the data container after the > remote get. The invalidation can't stop the remote get, all it does > is tell the caller that "Hey don't write the remote value you > retrieved into the L1." > Oh right, the get command will still use the value it got from the remote node, it just won't write it. That makes me wonder, though, if something like this can happen: 1. A invokes get(k), starts a L1ReadSynchronizer and a remote get to B 2. B invokes put(k, v) and sends an invalidation command to A 3. The invalidation command cancels the L1 put on A 4. A invokes get(k) again, finds the L1ReadSynchronizer from step 1) and queues on it 5. Both get(k) commands return the same value, even though the value has changed on the owner(s).

> > > > > We also have to remove the logic in AbstractLockingInterceptor that > > skips L1 > > invalidation for a key if it can't acquire a lock with a 0 timeout. > > > >> > >> 4. Local writes will also do the same thing as the invalidation with > >> cancelling or waiting. Note that non tx local writes only do L1 > >> invalidations and don't write the value to the data container. Reasons > >> why > >> I found at https://issues.jboss.org/browse/ISPN-3214 > > > > > > I didn't know about ISPN-3214 or that non-tx writes don't write to L1, > > but > > it sounds fair. > > Yeah I really wanted that to work, but without some additional checks > such as versioned data, I don't see a way to do this without locking > at the primary node like tx caches. > In theory, the primary owner could send a synchronous RPC back to the originator while it is holding the lock, saying "ok, you can now write the value to L1". But I don't think the slowdown from an additional RPC would be acceptable.

> > > > >> > >> 5. Writes that require the previous value and don't have it in the L1 > >> would also do it's get operations using the same "corralling" method. > > > > > > The remoteGetBeforeWrites are a bit different - they don't happen on > > non-owners, they only happen on writeCH-owners that didn't receive that > > entry via state transfer yet. They put the value in the > > InvocationContext, > > but they don't write it to the data container - nor do they invalidate > > the > > L1 entry, if it exists. > > Ah yes that is true, but only for non tx caches it seems. > Right, I wasn't considering the fact that a conditional command may fail... I think if that happens, even in non-tx caches EntryWrappingInterceptor may write the entry to the data container as an L1 entry. If we move the L1 writes to the L1 interceptor, we must ensure that EntryWrappingInterceptor doesn't write anything to L1 any more.

> > > > >> > >> > >> 4/5 are not currently implemented in PR. > >> > >> This approach would use no locking for non tx caches for all L1 > >> operations. The synchronization point would be done through the > >> "corralling" method and invalidations/writes communicating to it. > >> > >> Transactional caches would do almost the same thing as non-tx. Note > >> these > >> changes are not done in any way yet. > >> > >> 1. Gets would now update the L1 immediately after retrieving the value > >> without locking, but still using the "corralling" technique that non-tx > >> does. Previously the L1 update from a get was transactional. This > >> actually > >> would remedy issue [1] > >> > >> 2. Writes currently acquire the remote lock when committing, which is > >> why > >> tx caches are able to update the L1 with the value. Writes would do > >> the > >> same cancellation/wait method as non-tx. > >> Hmm, I don't think your current approach for L1 invalidations would work for L1 writes, because the actual write to the data container is not synchronized (well, technically you still have the 0-timeout locking for invalidation commands, but I think you're planning to remove that). So it's possible for an L1 write and an L1 invalidation to wait for the same remote get and then to get executed in the wrong order.

> > >> 3. Writes that require the previous value and don't have it in the L1 > >> would also do it's get operations using the same method. > > > > > > Just like for non-tx caches, I don't think these remote gets have to be > > stored in L1. > > Tx caches do the remote get and could cache the L1 value immediately. > This would help if the transaction is rolled back or a conditional > operation failed etc. There are some locking concerns, here but I > will leave that the other post. > Right, the L1 entry could be immediately written to the data container, but not if it complicates things too much: most writes should be successful anyway.

> > > >> > >> 4. For tx cache [2] would also have to be done. > >> > >> [1] - > >> > >> https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... > >> [2] - https://issues.jboss.org/browse/ISPN-1540 > >> > >> Also rehashing is another issue, but we should be able to acquire the > >> state transfer lock before updating the L1 on a get, just like when an > >> entry > >> is committed to the data container. > >> > > > > The same for L1 invalidations - we don't want to remove real entries > > from > > the data container after the local node became an owner. > > Yeah actually as you mentioned this, it sounds like a hole currently > even. I don't know if this case can happen, but what if you received > a L1 invalidation and then got a rehash event right before it was > committing it to the container? It seems the L1 commit to the > container would block until it could get the shared topology lock and > after it could then removes the value. I probably need to dig into > the state transfer stuff deeper to know myself. > I think that's ok, because the value is stale, and any future get/conditional write command will request the new value from the previous owners. I don't think the new value can arrive via a StateResponseCommand and be applied to the data container before the invalidation command manages to commit, because the put command sends the invalidations first (synchronously) and only then commits on the owner.

> > > > >> > >> Any comments/concerns would be appreciated. > >> > >> Thanks, > >> > >> - Will > >> > >> _______________________________________________ > >> 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

On Fri, Jun 28, 2013 at 4:39 PM, William Burns <mudokonman(a)gmail.com&gt; wrote: > > On Fri, Jun 28, 2013 at 5:14 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; > wrote: > > > > On Fri, Jun 28, 2013 at 12:17 AM, William Burns <mudokonman(a)gmail.com&gt; > > wrote: > >> > >> Trying to leave my points that would most likely have responses to > >> second email so we can try to get back to a single thread :) > >> > > > > No such luck :) > > > > Sorry for sending 2 replies in the first place, but it seemed more > > natural - > > I meant to comment on your proposal in one email and to describe my > > alternative proposal in the second email. > > > >> > >> On Thu, Jun 27, 2013 at 4:12 PM, Dan Berindei <dan.berindei(a)gmail.com&gt; > >> wrote: > >> > > >> > On Thu, Jun 27, 2013 at 4:18 PM, William Burns <mudokonman(a)gmail.com&gt; > >> > wrote: > >> >> > >> >> First off I apologize for the length. > >> >> > >> >> There have been a few Jiras recently that have identified L1 > >> >> consistency > >> >> issues with both TX and non TX sync caches. Async caches with L1 > >> >> have > >> >> their > >> >> own issues as well, but I only wanted to talk about sync caches. > >> >> > >> >> https://issues.jboss.org/browse/ISPN-3197 > >> >> https://issues.jboss.org/browse/ISPN-2965 > >> >> https://issues.jboss.org/browse/ISPN-2990 > >> >> > >> >> I have proposed a solution in > >> >> https://github.com/infinispan/infinispan/pull/1922 which should > >> >> start > >> >> L1 > >> >> consistency down the right track. There are quite a few comments on > >> >> it > >> >> if > >> >> you want to look into it more, but because of that I am moving this > >> >> to > >> >> the > >> >> dev mailing list. > >> >> > >> >> The key changes in the PR are the following (non-tx): > >> >> > >> >> 1. Concurrent reads for a key that can retrieve a remote value are > >> >> "corralled" into a single thread of execution for that given key. > >> >> This > >> >> would reduce network traffic with concurrent gets for the same key. > >> >> Note > >> >> the "corralling" only happens on a per key basis. > >> > > >> > > >> > Get commands on owners should not be serialized. Get commands on > >> > non-owners > >> > should not be serialized either, if the key already exists in L1. So > >> > I'd > >> > say > >> > L1ReadSynchronizer should be L1WriteSynchronizer instead :) > >> > >> You are suggesting to check the context to see if the key is present > >> before attempting the synchronizer right? Reading your second email > >> that seems the case :) > >> > > > > Nope, I meant we should check the data container (aka the L1 cache). But > > obviously we have to check the invocation context first in a tx cache, > > if > > the tx read the key before it should see the same value. > > > I was thinking because the non-tx wraps the value if it is in the data > container before hand. But actually for non-tx it seems I should > check the data container only and tx I should only check the ctx (to > guarantee read consistency) For tx you probably have to check the context first, and the data container only if you don't find the entry in the context (non-existent entries that were previously read in the tx should have a NullMarkerEntry in the context).

> >> > >> > > >> >> > >> >> 2. The single thread that is doing the remote get would update the > >> >> L1 > >> >> if > >> >> able (without locking) and make available the value to all the > >> >> requests > >> >> waiting on the get. > >> > > >> > > >> > Well, L1ReadSynchronizer does prevent other threads from modifying > >> > the > >> > same > >> > key, so we are locking the key - just not using LockManager. > >> > It would also require StateTransferLock.acquireSharedTopologyLock() > >> > to > >> > make > >> > sure it doesn't write an L1 entry after the node became a proper > >> > owner. > >> > >> Agree, when I was saying locking I was meaning through the use of the > >> lock manager. > >> > >> > > >> >> > >> >> 3. Invalidations that are received would first check to see if there > >> >> is > >> >> a > >> >> current remote get occurring for it's keys. If there is it will > >> >> attempt to > >> >> cancel the L1 write(s) before it occurs. If it cannot cancel the L1 > >> >> write, > >> >> then it must also wait on the current remote get completion and > >> >> subsequently > >> >> run the invalidation. Note the cancellation would fail when the > >> >> remote > >> >> get > >> >> was done and it is in the middle of updating the L1, so this would > >> >> be > >> >> very > >> >> small window. > >> > > >> > > >> > I think it would be clearer to describe this as the L1 invalidation > >> > cancelling the remote get, not the L1 update, because the actual L1 > >> > update > >> > can't be cancelled. > >> > >> When I say L1 update I meant the write to the data container after the > >> remote get. The invalidation can't stop the remote get, all it does > >> is tell the caller that "Hey don't write the remote value you > >> retrieved into the L1." > >> > > > > Oh right, the get command will still use the value it got from the > > remote > > node, it just won't write it. > > That makes me wonder, though, if something like this can happen: > > > > 1. A invokes get(k), starts a L1ReadSynchronizer and a remote get to B > > 2. B invokes put(k, v) and sends an invalidation command to A > > 3. The invalidation command cancels the L1 put on A > > 4. A invokes get(k) again, finds the L1ReadSynchronizer from step 1) and > > queues on it > > 5. Both get(k) commands return the same value, even though the value has > > changed on the owner(s). > > > > Yeah that was going to be covered in ISPN-2965. This would be a > problem even with locking since it could be a different node doing the > get. > > > https://github.com/wburns/infinispan/blob/ISPN-3197-singlethread/core/src... > > I also have a test currently that is disabled in the test class that > reproduces this. > > > https://github.com/wburns/infinispan/blob/ISPN-3197-singlethread/core/src... > I think we're talking about slightly different things here. Even if you wanted to send another invalidation command after the entry was committed to the data container on the owner, you won't get a second get request on the owner because that get request has been corralled on the requestor: 1st get initiated -> remote get sent -> invalidation -> remote get returns -> 2nd get initiated -> both get ops return with initial value

I think you need to either remove the L1ReadSynchronizer from the map during invalidation, or to re-fetch the value if there was an invalidation, in order to avoid this.

Of course, you still have to deal with ISPN-2965 and the fact that a get command can reach the owner after the invalidations were sent but before the entry is committed to the data container. Maybe it would be better if we'd send L1 invalidations only after the entry was committed?

> > > > >> > >> > > >> > We also have to remove the logic in AbstractLockingInterceptor that > >> > skips L1 > >> > invalidation for a key if it can't acquire a lock with a 0 timeout. > >> > > >> >> > >> >> 4. Local writes will also do the same thing as the invalidation with > >> >> cancelling or waiting. Note that non tx local writes only do L1 > >> >> invalidations and don't write the value to the data container. > >> >> Reasons > >> >> why > >> >> I found at https://issues.jboss.org/browse/ISPN-3214 > >> > > >> > > >> > I didn't know about ISPN-3214 or that non-tx writes don't write to > >> > L1, > >> > but > >> > it sounds fair. > >> > >> Yeah I really wanted that to work, but without some additional checks > >> such as versioned data, I don't see a way to do this without locking > >> at the primary node like tx caches. > >> > > > > In theory, the primary owner could send a synchronous RPC back to the > > originator while it is holding the lock, saying "ok, you can now write > > the > > value to L1". But I don't think the slowdown from an additional RPC > > would be > > acceptable. > > > > I would agree ;( > > >> > >> > > >> >> > >> >> 5. Writes that require the previous value and don't have it in the > >> >> L1 > >> >> would also do it's get operations using the same "corralling" > >> >> method. > >> > > >> > > >> > The remoteGetBeforeWrites are a bit different - they don't happen on > >> > non-owners, they only happen on writeCH-owners that didn't receive > >> > that > >> > entry via state transfer yet. They put the value in the > >> > InvocationContext, > >> > but they don't write it to the data container - nor do they > >> > invalidate > >> > the > >> > L1 entry, if it exists. > >> > >> Ah yes that is true, but only for non tx caches it seems. > >> > > > > Right, I wasn't considering the fact that a conditional command may > > fail... > > I think if that happens, even in non-tx caches EntryWrappingInterceptor > > may > > write the entry to the data container as an L1 entry. If we move the L1 > > writes to the L1 interceptor, we must ensure that > > EntryWrappingInterceptor > > doesn't write anything to L1 any more. > > > > We still need EntryWrappingInterceptor to write to the L1 cache for tx > caches I would think. We only want to write to the L1 after the tx is > committed for write commands. Read commands would be fine writing to > the container immediately. Also I would hope conditional writes > wouldn't update the context with the new value until we knew the write > was successful, should prevent bad data being written there. > The L1 interceptor could still only write to the data container during the commit phase. I was thinking of separating the regular writes from the L1 writes because L1 writes would need additional synchronization, but maybe that's not the case.

> > > > >> > >> > > >> >> > >> >> > >> >> 4/5 are not currently implemented in PR. > >> >> > >> >> This approach would use no locking for non tx caches for all L1 > >> >> operations. The synchronization point would be done through the > >> >> "corralling" method and invalidations/writes communicating to it. > >> >> > >> >> Transactional caches would do almost the same thing as non-tx. Note > >> >> these > >> >> changes are not done in any way yet. > >> >> > >> >> 1. Gets would now update the L1 immediately after retrieving the > >> >> value > >> >> without locking, but still using the "corralling" technique that > >> >> non-tx > >> >> does. Previously the L1 update from a get was transactional. This > >> >> actually > >> >> would remedy issue [1] > >> >> > >> >> 2. Writes currently acquire the remote lock when committing, which > >> >> is > >> >> why > >> >> tx caches are able to update the L1 with the value. Writes would do > >> >> the > >> >> same cancellation/wait method as non-tx. > >> >> > > > > > > Hmm, I don't think your current approach for L1 invalidations would work > > for > > L1 writes, because the actual write to the data container is not > > synchronized (well, technically you still have the 0-timeout locking for > > invalidation commands, but I think you're planning to remove that). So > > it's > > possible for an L1 write and an L1 invalidation to wait for the same > > remote > > get and then to get executed in the wrong order. > > > > For non-tx that doesn't matter as a write and L1 invalidation both do > invalidation. > > For tx it should be fine as well since it will hold the primary owner > lock for the write duration, which includes L1 writes (so only that tx > is writing it's value). Tx caches don't process the L1 invalidation > message when it comes back since they were the originator also. > True, in most cases different writers would be synchronized via the lock on the primary owner. There are 2 situations where that doesn't happen, though: * When syncCommitPhase = false, the primary lock may be released before the entry is actually committed to L1. * During state transfer, the received state is applied without holding a lock on the primary, or even a local lock. State transfer only uses StateConsumerImpl.updatedKeys for synchronization, which turns out to be a problem with regular entries as well: https://issues.jboss.org/browse/ISPN-3287

> > > > >> > >> >> 3. Writes that require the previous value and don't have it in the > >> >> L1 > >> >> would also do it's get operations using the same method. > >> > > >> > > >> > Just like for non-tx caches, I don't think these remote gets have to > >> > be > >> > stored in L1. > >> > >> Tx caches do the remote get and could cache the L1 value immediately. > >> This would help if the transaction is rolled back or a conditional > >> operation failed etc. There are some locking concerns, here but I > >> will leave that the other post. > >> > > > > Right, the L1 entry could be immediately written to the data container, > > but > > not if it complicates things too much: most writes should be successful > > anyway. > > > > I agree the writes should be successful, but there is no telling if > the transaction will be committed. Something else outside of > infinispan could error causing a rollback or at worst case 2PC could > fail prepare (not touching heuristic failures). > > Also this would help plug up this hole: > > https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... > Sorry, I wasn't clear... I meant if 99.9% of conditional writes write the initial value to L1 and then immediately write the new value, perhaps the additional overhead of an extra write to the data container is not worth the benefit of having the value in L1 in the 0.1% of cases where the write is not successful or the tx fails.

Of course, we don't have proper statistics about the percentage of conditional writes that succeed in real-life applications, so in the end it's your decision. > > >> > > >> >> > >> >> 4. For tx cache [2] would also have to be done. > >> >> > >> >> [1] - > >> >> > >> >> > >> >> https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... > >> >> [2] - https://issues.jboss.org/browse/ISPN-1540 > >> >> > >> >> Also rehashing is another issue, but we should be able to acquire > >> >> the > >> >> state transfer lock before updating the L1 on a get, just like when > >> >> an > >> >> entry > >> >> is committed to the data container. > >> >> > >> > > >> > The same for L1 invalidations - we don't want to remove real entries > >> > from > >> > the data container after the local node became an owner. > >> > >> Yeah actually as you mentioned this, it sounds like a hole currently > >> even. I don't know if this case can happen, but what if you received > >> a L1 invalidation and then got a rehash event right before it was > >> committing it to the container? It seems the L1 commit to the > >> container would block until it could get the shared topology lock and > >> after it could then removes the value. I probably need to dig into > >> the state transfer stuff deeper to know myself. > >> > > > > I think that's ok, because the value is stale, and any future > > get/conditional write command will request the new value from the > > previous > > owners. > > > > I don't think the new value can arrive via a StateResponseCommand and be > > applied to the data container before the invalidation command manages to > > commit, because the put command sends the invalidations first > > (synchronously) and only then commits on the owner. > > > > I defer to your judgement here ;-) > On a second read, that second paragraph doesn't make a lot of sense... To rephrase: the InvalidateL1Command could remove an entry after it was written via state transfer. Since the invalidation is synchronous, that means the commit command will be forwarded to the new owner, and it will write the new value gain. If any tx tries to read the key in the meantime, it will re-read it from the old owners, so that shouldn't be a problem. But if there are 2 old owners (let's call them A and B), the commit command forwarded from A could be executed on the new owner just before the invalidation command from B. And once the forwarded commit from A is executed, the forwarded commit from B for the same tx won't do anything, and k will be lost. Even if state transfer receives k after that, it won't overwrite it because it's in the StateConsumerImpl.updatedKeys set. There is another hitch with async commit - it's possible that the new owner didn't receive the transaction data via state transfer (because it requested transaction data from the B, and the transaction had already been committed there). Then any forwarded commit won't do anything and the entry will be lost. Going back to my proposal of adding an ownership check before actually removing the L1 entry (and while holding the shared topology lock)... Say nodes [A, B] were the previous owners of key k, C has k in its L1 cache, and [B, C] are the new owners (so the owners in the write CH are [A, B, C]). If there is a transaction modifying key k, the timing could be something like this: 1. The commit command finishes on A and B before they install the new topology. Because StateRequestCommands block until the state provider installs the new topology, that means node C didn't request the state for k yet, so the invalidateL1(k) command runs on C before the state transfer put(k, v) command. However, there is a chance that C already installed the new topology before executing the invalidate(k) command, so if we do just the ownership check in the L1 interceptor, the key won't be deleted. That's not ok, because we'll have a stale value until state transfer overwrites it. And even worse, if we had a remove(k) instead of a put(k, v), C would only remove the stale value for k when it expired. We may need to clear all the keys for which C is becoming an owner after installing the new topology, but before requesting transaction data (which means before processing transactions with the new topology id as well), when L1 is enabled.

2. The commit starts on A and B before the new topology is installed, but by the time it finishes on A, A installs the new topology. StateTransferInterceptor then forwards the commit command to C, and on C it writes the new value (eventually waiting for C to receive the transaction data from A). With sync commit + tx completion notification, C is guaranteed to receive the tx whether it requests it from A or B. If StateConsumer receives a value for k later, it won't overwrite it, except for this bug: https://issues.jboss.org/browse/ISPN-3287 There is a chance that the forwarded commit command from A reaches C around the same time with the invalidateL1(k) command from B. But the commit command is sent with the new topology id, so it will only execute after C installed the new topology as well. If the invalidateL1(k) command executes at the same time, the ownership check will prevent it from doing anything. (Without the ownership check we could remove key k just after writing it.) With async commit it's more complicated: if C requested the transaction data from B, and B had already finished committing by the time it sent transaction data, the forwarded commit on C won't do anything. Instead, C should receive the new value via state transfer, going back to case 1. 3. The commit is initiated after the new topology is installed. The commit command is sent to C directly, it waits there for C to receive transaction data from A or B, and it will write the new value. Since the commit was sent with the new topology id, it will block on A and B until they install the new topology, and won't trigger any invalidateL1(k) command to C. With async commit there is still a chance that A and/or B already finish the commit by the time C requests the transaction data, and C will only receive the new value via state transfer - just like in case 1. > > >> > >> > > >> >> > >> >> Any comments/concerns would be appreciated. > >> >> > >> >> Thanks, > >> >> > >> >> - Will > >> >> > >> >> _______________________________________________ > >> >> 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 > _______________________________________________ > 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

First off I apologize for the length. There have been a few Jiras recently that have identified L1 consistency issues with both TX and non TX sync caches. Async caches with L1 have their own issues as well, but I only wanted to talk about sync caches. https://issues.jboss.org/browse/ISPN-3197 https://issues.jboss.org/browse/ISPN-2965 https://issues.jboss.org/browse/ISPN-2990 I have proposed a solution in https://github.com/infinispan/infinispan/pull/1922 which should start L1 consistency down the right track. There are quite a few comments on it if you want to look into it more, but because of that I am moving this to the dev mailing list. The key changes in the PR are the following (non-tx): 1. Concurrent reads for a key that can retrieve a remote value are "corralled" into a single thread of execution for that given key. This would reduce network traffic with concurrent gets for the same key. Note the "corralling" only happens on a per key basis. 2. The single thread that is doing the remote get would update the L1 if able (without locking) and make available the value to all the requests waiting on the get. 3. Invalidations that are received would first check to see if there is a current remote get occurring for it's keys. If there is it will attempt to cancel the L1 write(s) before it occurs. If it cannot cancel the L1 write, then it must also wait on the current remote get completion and subsequently run the invalidation. Note the cancellation would fail when the remote get was done and it is in the middle of updating the L1, so this would be very small window.

4. Local writes will also do the same thing as the invalidation with cancelling or waiting. Note that non tx local writes only do L1 invalidations and don't write the value to the data container. Reasons why I found at https://issues.jboss.org/browse/ISPN-3214

5. Writes that require the previous value and don't have it in the L1 would also do it's get operations using the same "corralling" method. 4/5 are not currently implemented in PR. This approach would use no locking for non tx caches for all L1 operations. The synchronization point would be done through the "corralling" method and invalidations/writes communicating to it. Transactional caches would do almost the same thing as non-tx. Note these changes are not done in any way yet. 1. Gets would now update the L1 immediately after retrieving the value without locking, but still using the "corralling" technique that non-tx does. Previously the L1 update from a get was transactional. This actually would remedy issue [1] 2. Writes currently acquire the remote lock when committing, which is why tx caches are able to update the L1 with the value. Writes would do the same cancellation/wait method as non-tx. 3. Writes that require the previous value and don't have it in the L1 would also do it's get operations using the same method. 4. For tx cache [2] would also have to be done. [1] - https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&p... [2] - https://issues.jboss.org/browse/ISPN-1540 Also rehashing is another issue, but we should be able to acquire the state transfer lock before updating the L1 on a get, just like when an entry is committed to the data container. Any comments/concerns would be appreciated.