<br><div class="gmail_quote">On Fri, Jun 28, 2013 at 12:17 AM, William Burns <span dir="ltr"><<a href="mailto:mudokonman@gmail.com" target="_blank">mudokonman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Trying to leave my points that would most likely have responses to<br>
second email so we can try to get back to a single thread :)<br>
<div class="im"><br></div></blockquote><div><br>No such luck :)<br><br>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
On Thu, Jun 27, 2013 at 4:12 PM, Dan Berindei <<a href="mailto:dan.berindei@gmail.com">dan.berindei@gmail.com</a>> wrote:<br>
><br>
> On Thu, Jun 27, 2013 at 4:18 PM, William Burns <<a href="mailto:mudokonman@gmail.com">mudokonman@gmail.com</a>> wrote:<br>
>><br>
>> First off I apologize for the length.<br>
>><br>
>> There have been a few Jiras recently that have identified L1 consistency<br>
>> issues with both TX and non TX sync caches. Async caches with L1 have their<br>
>> own issues as well, but I only wanted to talk about sync caches.<br>
>><br>
>> <a href="https://issues.jboss.org/browse/ISPN-3197" target="_blank">https://issues.jboss.org/browse/ISPN-3197</a><br>
>> <a href="https://issues.jboss.org/browse/ISPN-2965" target="_blank">https://issues.jboss.org/browse/ISPN-2965</a><br>
>> <a href="https://issues.jboss.org/browse/ISPN-2990" target="_blank">https://issues.jboss.org/browse/ISPN-2990</a><br>
>><br>
>> I have proposed a solution in<br>
>> <a href="https://github.com/infinispan/infinispan/pull/1922" target="_blank">https://github.com/infinispan/infinispan/pull/1922</a> which should start L1<br>
>> consistency down the right track. There are quite a few comments on it if<br>
>> you want to look into it more, but because of that I am moving this to the<br>
>> dev mailing list.<br>
>><br>
>> The key changes in the PR are the following (non-tx):<br>
>><br>
>> 1. Concurrent reads for a key that can retrieve a remote value are<br>
>> "corralled" into a single thread of execution for that given key. This<br>
>> would reduce network traffic with concurrent gets for the same key. Note<br>
>> the "corralling" only happens on a per key basis.<br>
><br>
><br>
> Get commands on owners should not be serialized. Get commands on non-owners<br>
> should not be serialized either, if the key already exists in L1. So I'd say<br>
> L1ReadSynchronizer should be L1WriteSynchronizer instead :)<br>
<br>
</div>You are suggesting to check the context to see if the key is present<br>
before attempting the synchronizer right? Reading your second email<br>
that seems the case :)<br>
<div class="im"><br></div></blockquote><div><br>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
><br>
>><br>
>> 2. The single thread that is doing the remote get would update the L1 if<br>
>> able (without locking) and make available the value to all the requests<br>
>> waiting on the get.<br>
><br>
><br>
> Well, L1ReadSynchronizer does prevent other threads from modifying the same<br>
> key, so we are locking the key - just not using LockManager.<br>
> It would also require StateTransferLock.acquireSharedTopologyLock() to make<br>
> sure it doesn't write an L1 entry after the node became a proper owner.<br>
<br>
</div>Agree, when I was saying locking I was meaning through the use of the<br>
lock manager.<br>
<div class="im"><br></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
><br>
>><br>
>> 3. Invalidations that are received would first check to see if there is a<br>
>> current remote get occurring for it's keys. If there is it will attempt to<br>
>> cancel the L1 write(s) before it occurs. If it cannot cancel the L1 write,<br>
>> then it must also wait on the current remote get completion and subsequently<br>
>> run the invalidation. Note the cancellation would fail when the remote get<br>
>> was done and it is in the middle of updating the L1, so this would be very<br>
>> small window.<br>
><br>
><br>
> I think it would be clearer to describe this as the L1 invalidation<br>
> cancelling the remote get, not the L1 update, because the actual L1 update<br>
> can't be cancelled.<br>
<br>
</div>When I say L1 update I meant the write to the data container after the<br>
remote get. The invalidation can't stop the remote get, all it does<br>
is tell the caller that "Hey don't write the remote value you<br>
retrieved into the L1."<br>
<div class="im"><br></div></blockquote><div><br>Oh right, the get command will still use the value it got from the remote node, it just won't write it.<br>That makes me wonder, though, if something like this can happen:<br>
<br>1. A invokes get(k), starts a L1ReadSynchronizer and a remote get to B<br>2. B invokes put(k, v) and sends an invalidation command to A<br>3. The invalidation command cancels the L1 put on A<br>4. A invokes get(k) again, finds the L1ReadSynchronizer from step 1) and queues on it<br>
5. Both get(k) commands return the same value, even though the value has changed on the owner(s).<br><br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
><br>
> We also have to remove the logic in AbstractLockingInterceptor that skips L1<br>
> invalidation for a key if it can't acquire a lock with a 0 timeout.<br>
><br>
>><br>
>> 4. Local writes will also do the same thing as the invalidation with<br>
>> cancelling or waiting. Note that non tx local writes only do L1<br>
>> invalidations and don't write the value to the data container. Reasons why<br>
>> I found at <a href="https://issues.jboss.org/browse/ISPN-3214" target="_blank">https://issues.jboss.org/browse/ISPN-3214</a><br>
><br>
><br>
> I didn't know about ISPN-3214 or that non-tx writes don't write to L1, but<br>
> it sounds fair.<br>
<br>
</div>Yeah I really wanted that to work, but without some additional checks<br>
such as versioned data, I don't see a way to do this without locking<br>
at the primary node like tx caches.<br>
<div class="im"><br></div></blockquote><div><br>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
><br>
>><br>
>> 5. Writes that require the previous value and don't have it in the L1<br>
>> would also do it's get operations using the same "corralling" method.<br>
><br>
><br>
> The remoteGetBeforeWrites are a bit different - they don't happen on<br>
> non-owners, they only happen on writeCH-owners that didn't receive that<br>
> entry via state transfer yet. They put the value in the InvocationContext,<br>
> but they don't write it to the data container - nor do they invalidate the<br>
> L1 entry, if it exists.<br>
<br>
</div>Ah yes that is true, but only for non tx caches it seems.<br>
<div class="im"><br></div></blockquote><div><br>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.<br>
<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
><br>
>><br>
>><br>
>> 4/5 are not currently implemented in PR.<br>
>><br>
>> This approach would use no locking for non tx caches for all L1<br>
>> operations. The synchronization point would be done through the<br>
>> "corralling" method and invalidations/writes communicating to it.<br>
>><br>
>> Transactional caches would do almost the same thing as non-tx. Note these<br>
>> changes are not done in any way yet.<br>
>><br>
>> 1. Gets would now update the L1 immediately after retrieving the value<br>
>> without locking, but still using the "corralling" technique that non-tx<br>
>> does. Previously the L1 update from a get was transactional. This actually<br>
>> would remedy issue [1]<br>
>><br>
>> 2. Writes currently acquire the remote lock when committing, which is why<br>
>> tx caches are able to update the L1 with the value. Writes would do the<br>
>> same cancellation/wait method as non-tx.<br>
>><br></div></blockquote><div><br>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.<br>
<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
>> 3. Writes that require the previous value and don't have it in the L1<br>
>> would also do it's get operations using the same method.<br>
><br>
><br>
> Just like for non-tx caches, I don't think these remote gets have to be<br>
> stored in L1.<br>
<br>
</div>Tx caches do the remote get and could cache the L1 value immediately.<br>
This would help if the transaction is rolled back or a conditional<br>
operation failed etc. There are some locking concerns, here but I<br>
will leave that the other post.<br>
<div class="im"><br></div></blockquote><div><br>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.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
><br>
>><br>
>> 4. For tx cache [2] would also have to be done.<br>
>><br>
>> [1] -<br>
>> <a href="https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12779780" target="_blank">https://issues.jboss.org/browse/ISPN-2965?focusedCommentId=12779780&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12779780</a><br>
>> [2] - <a href="https://issues.jboss.org/browse/ISPN-1540" target="_blank">https://issues.jboss.org/browse/ISPN-1540</a><br>
>><br>
>> Also rehashing is another issue, but we should be able to acquire the<br>
>> state transfer lock before updating the L1 on a get, just like when an entry<br>
>> is committed to the data container.<br>
>><br>
><br>
> The same for L1 invalidations - we don't want to remove real entries from<br>
> the data container after the local node became an owner.<br>
<br>
</div>Yeah actually as you mentioned this, it sounds like a hole currently<br>
even. I don't know if this case can happen, but what if you received<br>
a L1 invalidation and then got a rehash event right before it was<br>
committing it to the container? It seems the L1 commit to the<br>
container would block until it could get the shared topology lock and<br>
after it could then removes the value. I probably need to dig into<br>
the state transfer stuff deeper to know myself.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div><br>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. <br>
<br>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><br>
>><br>
>> Any comments/concerns would be appreciated.<br>
>><br>
>> Thanks,<br>
>><br>
>> - Will<br>
>><br>
>> _______________________________________________<br>
>> infinispan-dev mailing list<br>
>> <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
><br>
><br>
><br>
> _______________________________________________<br>
> infinispan-dev mailing list<br>
> <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
_______________________________________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
</div></div></blockquote></div><br>