Hi, Re: https://issues.jboss.org/browse/ISPN-293 I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: - [main-thread] Put k0 in a cache that should own it. - [main-thread] Do a getAsync for k0 from a node that does not own it: - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks)

- [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :)

Cheers, -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hi, Re: https://issues.jboss.org/browse/ISPN-293 I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: - [main-thread] Put k0 in a cache that should own it. - [main-thread] Do a getAsync for k0 from a node that does not own it: - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException

Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) Cheers, -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

What about putting the entire call on a separate thread much earlier in the call stack? E.g., in the CacheDelegate? You get the added benefit of any cache loading happening offline as well. Plus a much simpler impl. :)

Sent from my iPhone On 9 Feb 2011, at 08:14, Galder Zamarreño <galder(a)redhat.com&gt; wrote: > Hi, > > Re: https://issues.jboss.org/browse/ISPN-293 > > I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: > > Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: > > - [main-thread] Put k0 in a cache that should own it. > - [main-thread] Do a getAsync for k0 from a node that does not own it: > - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) > - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): > - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException > > Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: > > if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { > > Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. > > I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) > > Cheers, > -- > Galder Zamarreño > Sr. Software Engineer > Infinispan, JBoss Cache > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

The problem that I see with that is getAsync() calls that should resolve locally will get the penalty of the thread context switching. I think I have a workable solution without having to rely on your suggestion which passes the test suite. I'll send a pull request and you (Mircea+Manik) can have a look to it, and then we can decide :) On Feb 9, 2011, at 2:32 PM, Mircea Markus wrote: > > On 9 Feb 2011, at 12:34, Manik Surtani wrote: > >> What about putting the entire call on a separate thread much earlier in the call stack? E.g., in the CacheDelegate? You get the added benefit of any cache loading happening offline as well. Plus a much simpler impl. :) > +1. >> >> Sent from my iPhone >> >> On 9 Feb 2011, at 08:14, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >> >>> Hi, >>> >>> Re: https://issues.jboss.org/browse/ISPN-293 >>> >>> I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: >>> >>> Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: >>> >>> - [main-thread] Put k0 in a cache that should own it. >>> - [main-thread] Do a getAsync for k0 from a node that does not own it: >>> - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) >>> - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): >>> - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException >>> >>> Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: >>> >>> if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { >>> >>> Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. >>> >>> I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) >>> >>> Cheers, >>> -- >>> Galder Zamarreño >>> Sr. Software Engineer >>> Infinispan, JBoss Cache >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 9 Feb 2011, at 17:35, Galder Zamarreño wrote: > Hmmmm, I've thought this further. The penalty of the thread ctx switching is probably not that bad actually cos the point of getAsync() is the ability to paralelise rather than how quickly getAsync() returns. Or a local get can be forced(flag) before giving control(if local value doesn't exist) to the new thread. > > On Feb 9, 2011, at 4:38 PM, Galder Zamarreño wrote: > >> The problem that I see with that is getAsync() calls that should resolve locally will get the penalty of the thread context switching. >> >> I think I have a workable solution without having to rely on your suggestion which passes the test suite. I'll send a pull request and you (Mircea+Manik) can have a look to it, and then we can decide :) >> >> On Feb 9, 2011, at 2:32 PM, Mircea Markus wrote: >> >>> >>> On 9 Feb 2011, at 12:34, Manik Surtani wrote: >>> >>>> What about putting the entire call on a separate thread much earlier in the call stack? E.g., in the CacheDelegate? You get the added benefit of any cache loading happening offline as well. Plus a much simpler impl. :) >>> +1. >>>> >>>> Sent from my iPhone >>>> >>>> On 9 Feb 2011, at 08:14, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >>>> >>>>> Hi, >>>>> >>>>> Re: https://issues.jboss.org/browse/ISPN-293 >>>>> >>>>> I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: >>>>> >>>>> Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: >>>>> >>>>> - [main-thread] Put k0 in a cache that should own it. >>>>> - [main-thread] Do a getAsync for k0 from a node that does not own it: >>>>> - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) >>>>> - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): >>>>> - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException >>>>> >>>>> Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: >>>>> >>>>> if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { >>>>> >>>>> Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. >>>>> >>>>> I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) >>>>> >>>>> Cheers, >>>>> -- >>>>> Galder Zamarreño >>>>> Sr. Software Engineer >>>>> Infinispan, JBoss Cache >>>>> >>>>> >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Galder Zamarreño > Sr. Software Engineer > Infinispan, JBoss Cache > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

How do other *Async() calls deal with jta, if at all? On 10 Feb 2011, at 11:10, Galder Zamarreño wrote: > That's a good optimisation Mircea, I've added it. > > There's another important problem to solve here which is, how do you make getAsync() maintain the transaction semantics of the incoming thread? getAsync does not acquire locks except in the case where the remote get leads to a L1 put in which case a write lock is acquired on entry put on L1. > > The main problem here is how to make the transaction propagate from one thread to the other? I tried to pass the current thread's transaction to the getAsync() thread and create an invocation context using it. This doesn't work cos LocalTxInvocationContext does not keep the current transaction, instead it keeps the localTransaction which for 1st invocation is null. > > The Infinispan code relies on TransactionManager.getTransaction() which most likely relies on a ThreadLocal being set to the current transaction. Once you switch threads, this method returns null, and with the solution attempted solution I get: > > Caused by: java.lang.IllegalStateException: This should only be called in an tx scope > at org.infinispan.interceptors.TxInterceptor.enlist(TxInterceptor.java:193) > at org.infinispan.interceptors.TxInterceptor.enlistReadAndInvokeNext(TxInterceptor.java:167) > at org.infinispan.interceptors.TxInterceptor.visitGetKeyValueCommand(TxInterceptor.java:162) > > So, what I've done is get the InvocationContextContainer to take the passed transaction and assign it to the local tx invocation context, and then use this transaction rather than the one from tm.getTransaction() in the enlist method. > > I've created a pull request with this solution and you can find it in: https://github.com/infinispan/infinispan/pull/160 > > Note that I've made a couple of TODOs in AbstractTxInvocationContext in order to discuss potentially naming getTransaction/setTransaction differently and see if we wanna merge somehow with getRunningTransaction() available in TxInvocationContext. > > Cheers, > > On Feb 9, 2011, at 6:42 PM, Mircea Markus wrote: > >> On 9 Feb 2011, at 17:35, Galder Zamarreño wrote: >> >>> Hmmmm, I've thought this further. The penalty of the thread ctx switching is probably not that bad actually cos the point of getAsync() is the ability to paralelise rather than how quickly getAsync() returns. >> Or a local get can be forced(flag) before giving control(if local value doesn't exist) to the new thread. >>> >>> On Feb 9, 2011, at 4:38 PM, Galder Zamarreño wrote: >>> >>>> The problem that I see with that is getAsync() calls that should resolve locally will get the penalty of the thread context switching. >>>> >>>> I think I have a workable solution without having to rely on your suggestion which passes the test suite. I'll send a pull request and you (Mircea+Manik) can have a look to it, and then we can decide :) >>>> >>>> On Feb 9, 2011, at 2:32 PM, Mircea Markus wrote: >>>> >>>>> >>>>> On 9 Feb 2011, at 12:34, Manik Surtani wrote: >>>>> >>>>>> What about putting the entire call on a separate thread much earlier in the call stack? E.g., in the CacheDelegate? You get the added benefit of any cache loading happening offline as well. Plus a much simpler impl. :) >>>>> +1. >>>>>> >>>>>> Sent from my iPhone >>>>>> >>>>>> On 9 Feb 2011, at 08:14, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Re: https://issues.jboss.org/browse/ISPN-293 >>>>>>> >>>>>>> I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: >>>>>>> >>>>>>> Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: >>>>>>> >>>>>>> - [main-thread] Put k0 in a cache that should own it. >>>>>>> - [main-thread] Do a getAsync for k0 from a node that does not own it: >>>>>>> - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) >>>>>>> - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): >>>>>>> - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException >>>>>>> >>>>>>> Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: >>>>>>> >>>>>>> if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { >>>>>>> >>>>>>> Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. >>>>>>> >>>>>>> I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) >>>>>>> >>>>>>> Cheers, >>>>>>> -- >>>>>>> Galder Zamarreño >>>>>>> Sr. Software Engineer >>>>>>> Infinispan, JBoss Cache >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> infinispan-dev mailing list >>>>>>> infinispan-dev(a)lists.jboss.org >>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>>> >>>>>> _______________________________________________ >>>>>> infinispan-dev mailing list >>>>>> infinispan-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>> >>>>> >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>> >>>> -- >>>> Galder Zamarreño >>>> Sr. Software Engineer >>>> Infinispan, JBoss Cache >>>> >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> -- >>> Galder Zamarreño >>> Sr. Software Engineer >>> Infinispan, JBoss Cache >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Galder Zamarreño > Sr. Software Engineer > Infinispan, JBoss Cache > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

2011/2/11 Galder Zamarreño <galder(a)redhat.com&gt;: > The other *Async() limit themselves to making the RPC call in a separate thread, i.e. putAsync. > > So, by the time they have to go remote to replicate something, they've already acquire the locks within the user thread and the transaction is in prepare state. > > The PrepareCommand should contains a GlobalTransaction which transforms into a RemoteTransaction in the other node. > > I don't see the need to transfer actual javax.transaction.Transaction information with these commands. So basically if I abort a transaction during which I performed some asyncPut() operations, they are not rolled back? I could live with that, as long as we clearly document this.

> > Btw, I've just been talking to Mircea and we're considering passing around the LocalTransaction between the caller and the getAsync thread rather than passing javax.transaction.Transaction which we don't have control over. Jonathan mentioned a way to share transactions across threads using JBoss TransactionManager;

the main problem with this is that while something similar might be supported by others: it's not standardized. > > On Feb 10, 2011, at 7:09 PM, Manik Surtani wrote: > >> How do other *Async() calls deal with jta, if at all? >> >> On 10 Feb 2011, at 11:10, Galder Zamarreño wrote: >> >>> That's a good optimisation Mircea, I've added it. >>> >>> There's another important problem to solve here which is, how do you make getAsync() maintain the transaction semantics of the incoming thread? getAsync does not acquire locks except in the case where the remote get leads to a L1 put in which case a write lock is acquired on entry put on L1. >>> >>> The main problem here is how to make the transaction propagate from one thread to the other? I tried to pass the current thread's transaction to the getAsync() thread and create an invocation context using it. This doesn't work cos LocalTxInvocationContext does not keep the current transaction, instead it keeps the localTransaction which for 1st invocation is null. >>> >>> The Infinispan code relies on TransactionManager.getTransaction() which most likely relies on a ThreadLocal being set to the current transaction. Once you switch threads, this method returns null, and with the solution attempted solution I get: >>> >>> Caused by: java.lang.IllegalStateException: This should only be called in an tx scope >>> at org.infinispan.interceptors.TxInterceptor.enlist(TxInterceptor.java:193) >>> at org.infinispan.interceptors.TxInterceptor.enlistReadAndInvokeNext(TxInterceptor.java:167) >>> at org.infinispan.interceptors.TxInterceptor.visitGetKeyValueCommand(TxInterceptor.java:162) >>> >>> So, what I've done is get the InvocationContextContainer to take the passed transaction and assign it to the local tx invocation context, and then use this transaction rather than the one from tm.getTransaction() in the enlist method. >>> >>> I've created a pull request with this solution and you can find it in: https://github.com/infinispan/infinispan/pull/160 >>> >>> Note that I've made a couple of TODOs in AbstractTxInvocationContext in order to discuss potentially naming getTransaction/setTransaction differently and see if we wanna merge somehow with getRunningTransaction() available in TxInvocationContext. >>> >>> Cheers, >>> >>> On Feb 9, 2011, at 6:42 PM, Mircea Markus wrote: >>> >>>> On 9 Feb 2011, at 17:35, Galder Zamarreño wrote: >>>> >>>>> Hmmmm, I've thought this further. The penalty of the thread ctx switching is probably not that bad actually cos the point of getAsync() is the ability to paralelise rather than how quickly getAsync() returns. >>>> Or a local get can be forced(flag) before giving control(if local value doesn't exist) to the new thread. >>>>> >>>>> On Feb 9, 2011, at 4:38 PM, Galder Zamarreño wrote: >>>>> >>>>>> The problem that I see with that is getAsync() calls that should resolve locally will get the penalty of the thread context switching. >>>>>> >>>>>> I think I have a workable solution without having to rely on your suggestion which passes the test suite. I'll send a pull request and you (Mircea+Manik) can have a look to it, and then we can decide :) >>>>>> >>>>>> On Feb 9, 2011, at 2:32 PM, Mircea Markus wrote: >>>>>> >>>>>>> >>>>>>> On 9 Feb 2011, at 12:34, Manik Surtani wrote: >>>>>>> >>>>>>>> What about putting the entire call on a separate thread much earlier in the call stack? E.g., in the CacheDelegate? You get the added benefit of any cache loading happening offline as well. Plus a much simpler impl. :) >>>>>>> +1. >>>>>>>> >>>>>>>> Sent from my iPhone >>>>>>>> >>>>>>>> On 9 Feb 2011, at 08:14, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> Re: https://issues.jboss.org/browse/ISPN-293 >>>>>>>>> >>>>>>>>> I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: >>>>>>>>> >>>>>>>>> Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: >>>>>>>>> >>>>>>>>> - [main-thread] Put k0 in a cache that should own it. >>>>>>>>> - [main-thread] Do a getAsync for k0 from a node that does not own it: >>>>>>>>> - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) >>>>>>>>> - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): >>>>>>>>> - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException >>>>>>>>> >>>>>>>>> Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: >>>>>>>>> >>>>>>>>> if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { >>>>>>>>> >>>>>>>>> Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. >>>>>>>>> >>>>>>>>> I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) >>>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> -- >>>>>>>>> Galder Zamarreño >>>>>>>>> Sr. Software Engineer >>>>>>>>> Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

2011/2/11 Galder Zamarreño <galder(a)redhat.com&gt;: > The other *Async() limit themselves to making the RPC call in a separate thread, i.e. putAsync. > > So, by the time they have to go remote to replicate something, they've already acquire the locks within the user thread and the transaction is in prepare state. > > The PrepareCommand should contains a GlobalTransaction which transforms into a RemoteTransaction in the other node. > > I don't see the need to transfer actual javax.transaction.Transaction information with these commands. So basically if I abort a transaction during which I performed some asyncPut() operations, they are not rolled back? I could live with that, as long as we clearly document this.

> > Btw, I've just been talking to Mircea and we're considering passing around the LocalTransaction between the caller and the getAsync thread rather than passing javax.transaction.Transaction which we don't have control over. Jonathan mentioned a way to share transactions across threads using JBoss TransactionManager; the main problem with this is that while something similar might be supported by others: it's not standardized. > > On Feb 10, 2011, at 7:09 PM, Manik Surtani wrote: > >> How do other *Async() calls deal with jta, if at all? >> >> On 10 Feb 2011, at 11:10, Galder Zamarreño wrote: >> >>> That's a good optimisation Mircea, I've added it. >>> >>> There's another important problem to solve here which is, how do you make getAsync() maintain the transaction semantics of the incoming thread? getAsync does not acquire locks except in the case where the remote get leads to a L1 put in which case a write lock is acquired on entry put on L1. >>> >>> The main problem here is how to make the transaction propagate from one thread to the other? I tried to pass the current thread's transaction to the getAsync() thread and create an invocation context using it. This doesn't work cos LocalTxInvocationContext does not keep the current transaction, instead it keeps the localTransaction which for 1st invocation is null. >>> >>> The Infinispan code relies on TransactionManager.getTransaction() which most likely relies on a ThreadLocal being set to the current transaction. Once you switch threads, this method returns null, and with the solution attempted solution I get: >>> >>> Caused by: java.lang.IllegalStateException: This should only be called in an tx scope >>> at org.infinispan.interceptors.TxInterceptor.enlist(TxInterceptor.java:193) >>> at org.infinispan.interceptors.TxInterceptor.enlistReadAndInvokeNext(TxInterceptor.java:167) >>> at org.infinispan.interceptors.TxInterceptor.visitGetKeyValueCommand(TxInterceptor.java:162) >>> >>> So, what I've done is get the InvocationContextContainer to take the passed transaction and assign it to the local tx invocation context, and then use this transaction rather than the one from tm.getTransaction() in the enlist method. >>> >>> I've created a pull request with this solution and you can find it in: https://github.com/infinispan/infinispan/pull/160 >>> >>> Note that I've made a couple of TODOs in AbstractTxInvocationContext in order to discuss potentially naming getTransaction/setTransaction differently and see if we wanna merge somehow with getRunningTransaction() available in TxInvocationContext. >>> >>> Cheers, >>> >>> On Feb 9, 2011, at 6:42 PM, Mircea Markus wrote: >>> >>>> On 9 Feb 2011, at 17:35, Galder Zamarreño wrote: >>>> >>>>> Hmmmm, I've thought this further. The penalty of the thread ctx switching is probably not that bad actually cos the point of getAsync() is the ability to paralelise rather than how quickly getAsync() returns. >>>> Or a local get can be forced(flag) before giving control(if local value doesn't exist) to the new thread. >>>>> >>>>> On Feb 9, 2011, at 4:38 PM, Galder Zamarreño wrote: >>>>> >>>>>> The problem that I see with that is getAsync() calls that should resolve locally will get the penalty of the thread context switching. >>>>>> >>>>>> I think I have a workable solution without having to rely on your suggestion which passes the test suite. I'll send a pull request and you (Mircea+Manik) can have a look to it, and then we can decide :) >>>>>> >>>>>> On Feb 9, 2011, at 2:32 PM, Mircea Markus wrote: >>>>>> >>>>>>> >>>>>>> On 9 Feb 2011, at 12:34, Manik Surtani wrote: >>>>>>> >>>>>>>> What about putting the entire call on a separate thread much earlier in the call stack? E.g., in the CacheDelegate? You get the added benefit of any cache loading happening offline as well. Plus a much simpler impl. :) >>>>>>> +1. >>>>>>>> >>>>>>>> Sent from my iPhone >>>>>>>> >>>>>>>> On 9 Feb 2011, at 08:14, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> Re: https://issues.jboss.org/browse/ISPN-293 >>>>>>>>> >>>>>>>>> I have an issue with my implementation that simply wraps the realRemoteGet in DistributionInterceptor around a Callable: >>>>>>>>> >>>>>>>>> Assume that cache is configured with Distribution(numOwners=1, l1=enabled), no transactions, and we have a cluster of 2 nodes: >>>>>>>>> >>>>>>>>> - [main-thread] Put k0 in a cache that should own it. >>>>>>>>> - [main-thread] Do a getAsync for k0 from a node that does not own it: >>>>>>>>> - [async-thread] This leads to a remote get which updates the L1 and updates the context created by the main-thread and putting the updated entry in there (however, this thread does not release the locks) >>>>>>>>> - [main-thread] Next up, we put a new key, i.e. k1, in the node that didn't own k0 (the node where we updated the L1 cache): >>>>>>>>> - Now this thread has used the same context that the async thread used, so when it comes to releasing locks, it finds two entries in the context, the one locked by the async-thread and one for the main-thread and it fails with java.lang.IllegalMonitorStateException >>>>>>>>> >>>>>>>>> Now, what's happening here is the async-thread is acquiring the lock but not releasing it because the release happens in the DistLockInterceptor/LockingInterceptor, so a different interceptor to where the lock is being acquired. So, in theory, the solution would be for DistLockInterceptor to wrap the invokeNext() and afterCall() for when an async get so that all "remote get", "l1 update", and "release locks", happen in a separate thread. However, for this to work, it will need to figure out whether a remoteGet() will be necessary in the first place, otherwise is useless. Whether the remoteGet should happen is determined by this code in DistributionInterceptor: >>>>>>>>> >>>>>>>>> if (ctx.isOriginLocal() && !(isMappedToLocalNode = dm.isLocal(key)) && isNotInL1(key)) { >>>>>>>>> >>>>>>>>> Also, if DistLockInterceptor does this check, we need to make sure DistributionInterceptor does not do it again, otherwise it's a waste. I think this might work although it does require some further reengineering. >>>>>>>>> >>>>>>>>> I'm gonna try to implement this but wondering whether anyone can see any potential flaws here, or if anyone has any better ideas :) >>>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> -- >>>>>>>>> Galder Zamarreño >>>>>>>>> Sr. Software Engineer >>>>>>>>> Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

s/so/no: So if u rollback, no remote puts are done.