<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 17, 2013 at 11:11 AM, Dan Berindei <span dir="ltr"><<a href="mailto:dan.berindei@gmail.com" target="_blank">dan.berindei@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Mon, Jun 17, 2013 at 3:58 PM, Pedro Ruivo <span dir="ltr"><<a href="mailto:pedro@infinispan.org" target="_blank">pedro@infinispan.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><br>
<br>
On 06/17/2013 12:56 PM, Mircea Markus wrote:<br>
><br>
> On 17 Jun 2013, at 11:52, Pedro Ruivo <<a href="mailto:pedro@infinispan.org" target="_blank">pedro@infinispan.org</a>> wrote:<br>
><br>
>> I've been looking at TxDistributionInterceptor and I have a couple of<br>
>> questions (assuming REPEATABLE_READ isolation level):<br>
>><br>
>> #1. why are we doing a remote get each time we write on a key? (huge<br>
>> perform impact if the key was previously read)<br>
> indeed this is suboptimal for transactions that write the same key repeatedly and repeatable read. Can you please create a JIRA for this?<br>
<br>
</div>created: <a href="https://issues.jboss.org/browse/ISPN-3235" target="_blank">https://issues.jboss.org/browse/ISPN-3235</a><br>
<div><br></div></blockquote><div><br></div></div><div>Oops... when I fixed <a href="https://issues.jboss.org/browse/ISPN-3124" target="_blank">https://issues.jboss.org/browse/ISPN-3124</a> I removed the SKIP_REMOTE_LOOKUP, thinking that the map is already in the invocation context so there shouldn't be any perf penalty. I can't put the SKIP_REMOTE_LOOKUP flag back, otherwise delta writes won't have the previous value during state transfer, so +1 to fixing ISPN-3235.<br>
<br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div>
>><br>
>> #2. why are we doing a dataContainer.get() if the remote get returns a<br>
>> null value? Shouldn't the interactions with data container be performed<br>
>> only in the (Versioned)EntryWrappingInterceptor?<br>
> This was added in the scope of ISPN-2688 and covers the scenario in which a state transfer is in progress, the remote get returns null as the remote value was dropped (no longer owner) and this node has become the owner in between.<br>
><br>
<br>
</div>ok :)<br>
<div><br></div></blockquote><div><br></div></div><div>Yeah, this should be correct as long as we check if we already have the key in the invocation context before doing the remote + local get.<br><br></div><div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div>
>><br>
>> #3. (I didn't verify this) why are we acquire the lock is the remote get<br>
>> is performed for a write? This looks correct for pessimistic locking but<br>
>> not for optimistic...<br>
> I think that, given that the local node is not owner, the lock acquisition is redundant even for pessimistic caches.<br>
> Mind creating a test to check if dropping that lock acquisition doesn't break things?<br>
<br>
</div>I created a JIRA with low priority since it does not affect the<br>
transaction outcome/isolation and I believe the performance impact<br>
should be lower (you can increase the priority if you want).<br>
<br>
<a href="https://issues.jboss.org/browse/ISPN-3237" target="_blank">https://issues.jboss.org/browse/ISPN-3237</a><br></blockquote><div><br></div></div><div>If we don't lock the L1 entry, I think something like this could happen:<br>
<br></div><div>tx1@A: remote get(k1) from B - stores k1=v1 in invocation context<br></div><div>tx2@A: write(k1, v2)<br></div><div>tx2@A: commit - writes k1=v2 in L1<br></div><div>tx1@A: commit - overwrites k1=v1 in L1<br>
</div></div></div></div></blockquote><div>This one is just like here: referenced in <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></div>
<div><br></div><div>And even locking doesn't help in this case since it doesn't lock the key for a remote get only a remote get in the context of a write - which means the L1 could be updated concurrently in either order - causing possibly an inconsistency. This will be solved when I port the same fix I have for <a href="https://issues.jboss.org/browse/ISPN-3197" target="_blank">https://issues.jboss.org/browse/ISPN-3197</a> for tx caches.</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<br></div><div><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>>><br>
>> After this analysis, it is possible to break the isolation between<br>
>> transaction if I do a get on the key that does not exist:<br>
>><br>
>> tm.begin()<br>
>> cache.get(k) //returns null<br>
>> //in the meanwhile a transaction writes on k and commits<br>
>> cache.get(k) //return the new value. IMO, this is not valid for<br>
>> REPEATABLE_READ isolation level!<br>
><br>
> Indeed sounds like a bug, well spotted.<br>
> Can you please add a UT to confirm it and raise a JIRA?<br>
<br>
</div>created: <a href="https://issues.jboss.org/browse/ISPN-3236" target="_blank">https://issues.jboss.org/browse/ISPN-3236</a><br>
<br>
IMO, this should be the correct behaviour (I'm going to add the test<br>
cases later):<br>
<br>
tm.begin()<br>
cache.get(k) //returns null (op#1)<br>
<div>//in the meanwhile a transaction writes on k and commits<br>
</div>write operation performed:<br>
* put: must return the same value as op#1<br>
* conditional put //if op#1 returns null the operation should be always<br>
successful (i.e. the key is updated, return true). Otherwise, the key<br>
remains unchanged (return false)<br>
* replace: must return the same value as op#1<br>
* conditional replace: replace should be successful if checked with the<br>
op#1 return value (return true). Otherwise, the key must remain<br>
unchanged (return false).<br>
* remote: must return the same value as op#1<br>
* conditional remove: the key should be removed if checked with the op#1<br>
return value (return true). Otherwise, the key must remain unchanged<br>
(return false)<br>
<br>
Also, the description above should be valid after a removal of a key.<br>
<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
><br>
> Cheers,<br>
<div><div>><br>
_______________________________________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">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><br></div></div></div></div></div>
<br>_______________________________________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">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></blockquote></div><br></div></div>