On Mon, Jun 17, 2013 at 3:58 PM, Pedro Ruivo <[email protected]> wrote:
> > > On 06/17/2013 12:56 PM, Mircea Markus wrote: > > > > On 17 Jun 2013, at 11:52, Pedro Ruivo <[email protected]> wrote: > > > >> I've been looking at TxDistributionInterceptor and I have a couple of > >> questions (assuming REPEATABLE_READ isolation level): > >> > >> #1. why are we doing a remote get each time we write on a key? (huge > >> perform impact if the key was previously read) > > indeed this is suboptimal for transactions that write the same key > repeatedly and repeatable read. Can you please create a JIRA for this? > > created: https://issues.jboss.org/browse/ISPN-3235 > > Oops... when I fixed https://issues.jboss.org/browse/ISPN-3124 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. > >> > >> #2. why are we doing a dataContainer.get() if the remote get returns a > >> null value? Shouldn't the interactions with data container be performed > >> only in the (Versioned)EntryWrappingInterceptor? > > 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. > > > > ok :) > > 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. > >> > >> #3. (I didn't verify this) why are we acquire the lock is the remote get > >> is performed for a write? This looks correct for pessimistic locking but > >> not for optimistic... > > I think that, given that the local node is not owner, the lock > acquisition is redundant even for pessimistic caches. > > Mind creating a test to check if dropping that lock acquisition doesn't > break things? > > I created a JIRA with low priority since it does not affect the > transaction outcome/isolation and I believe the performance impact > should be lower (you can increase the priority if you want). > > https://issues.jboss.org/browse/ISPN-3237 > If we don't lock the L1 entry, I think something like this could happen: tx1@A: remote get(k1) from B - stores k1=v1 in invocation context tx2@A: write(k1, v2) tx2@A: commit - writes k1=v2 in L1 tx1@A: commit - overwrites k1=v1 in L1 >> > >> After this analysis, it is possible to break the isolation between > >> transaction if I do a get on the key that does not exist: > >> > >> tm.begin() > >> cache.get(k) //returns null > >> //in the meanwhile a transaction writes on k and commits > >> cache.get(k) //return the new value. IMO, this is not valid for > >> REPEATABLE_READ isolation level! > > > > Indeed sounds like a bug, well spotted. > > Can you please add a UT to confirm it and raise a JIRA? > > created: https://issues.jboss.org/browse/ISPN-3236 > > IMO, this should be the correct behaviour (I'm going to add the test > cases later): > > tm.begin() > cache.get(k) //returns null (op#1) > //in the meanwhile a transaction writes on k and commits > write operation performed: > * put: must return the same value as op#1 > * conditional put //if op#1 returns null the operation should be always > successful (i.e. the key is updated, return true). Otherwise, the key > remains unchanged (return false) > * replace: must return the same value as op#1 > * conditional replace: replace should be successful if checked with the > op#1 return value (return true). Otherwise, the key must remain > unchanged (return false). > * remote: must return the same value as op#1 > * conditional remove: the key should be removed if checked with the op#1 > return value (return true). Otherwise, the key must remain unchanged > (return false) > > Also, the description above should be valid after a removal of a key. > > > > > Cheers, > > > _______________________________________________ > infinispan-dev mailing list > [email protected] > https://lists.jboss.org/mailman/listinfo/infinispan-dev >
_______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
