[infinispan-dev] Atomic operations and transactions

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
Hello all,
some team members had a meeting yesterday, one of the discussed
subjects was about using atomic operations (putIfAbsent, etc..).
Mircea just summarised it in the following proposal:

The atomic operations, as defined by the ConcurrentHashMap, don't fit
well within the scope of optimistic transaction: this is because there
is a discrepancy between the value returned by the operation and the
value and the fact that the operation is applied or not:
E.g. putIfAbsent(k, v) might return true as there's no entry for k in
the scope of the current transaction, but in fact there might be a
value committed by another transaction, hidden by the fact we're
running in repeatable read mode.
Later on, at prepare time when the same operation is applied on the
node that actually holds k, it might not succeed as another
transaction has updated k in between, but the return value of the
method was already evaluated long before this point.
In order to solve this problem, if an atomic operations happens within
the scope of a transaction, Infinispan eagerly acquires a lock on the
remote node. This locks is held for the entire duration of the
transaction, and is an expensive lock as it involves an RPC. If
keeping the lock remotely for potentially long time represents a
problem, the user can suspend the running transaction and run the
atomic operation out of transaction's scope, then resume the
transaction.


In addition to this, would would you think about adding a flag to
these methods which acts as suspending the transaction just before and
resuming it right after? I don't know what is the cost of suspending &
resuming a transaction, but such a flag could optionally be optimized
in future by just ignoring the current transaction instead of really
suspending it, or apply other clever tricks we might come across.

I also think that we should discuss if such a behaviour should not be
the default - anybody using an atomic operation is going to make some
assumptions which are clearly incompatible with the transaction, so
I'm wondering what is the path here to "least surprise" for default
invocation.

Regards,
Sanne
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Mircea Markus

On 30 Jun 2011, at 14:11, Sanne Grinovero wrote:

Hello all,
some team members had a meeting yesterday, one of the discussed
subjects was about using atomic operations (putIfAbsent, etc..).
Mircea just summarised it in the following proposal:

The atomic operations, as defined by the ConcurrentHashMap, don't fit
well within the scope of optimistic transaction: this is because there
is a discrepancy between the value returned by the operation and the
value and the fact that the operation is applied or not:
E.g. putIfAbsent(k, v) might return true as there's no entry for k in
the scope of the current transaction, but in fact there might be a
value committed by another transaction, hidden by the fact we're
running in repeatable read mode.
Later on, at prepare time when the same operation is applied on the
node that actually holds k, it might not succeed as another
transaction has updated k in between, but the return value of the
method was already evaluated long before this point.
In order to solve this problem, if an atomic operations happens within
the scope of a transaction, Infinispan eagerly acquires a lock on the
remote node. This locks is held for the entire duration of the
transaction, and is an expensive lock as it involves an RPC. If
keeping the lock remotely for potentially long time represents a
problem, the user can suspend the running transaction and run the
atomic operation out of transaction's scope, then resume the
transaction.
Just to make this clearer, optimistic locking is what we'll get my only acquiring locks at prepare time:http://community.jboss.org/wiki/OptimisticLockingInInfinispan 


In addition to this, would would you think about adding a flag to
these methods which acts as suspending the transaction just before and
resuming it right after? I don't know what is the cost of suspending &
resuming a transaction,
I'd expect it to be no more than a remove from a ThreadLocal.
but such a flag could optionally be optimized
in future by just ignoring the current transaction instead of really
suspending it, or apply other clever tricks we might come across.
The only reason I see for such a flag is to make the code easier to write. AFAIK you're the only user for this functionality ATM, so if it makes your life easier, i.e. it makes you write considerably fewer lines of code then +1.
OTOH if it's only few places where you make use of it, then I'd rather not add it now - but only in future when there's more demand for this functionality.   

I also think that we should discuss if such a behaviour should not be
the default - anybody using an atomic operation is going to make some
assumptions which are clearly incompatible with the transaction,
The behaviour I described, whilst costly, is not incompatible with the transaction, so should not cause any surprise.
so
I'm wondering what is the path here to "least surprise" for default
invocation.


Regards,
Sanne
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Galder Zamarreno
In reply to this post by Sanne Grinovero-3
Do these atomic operations really make sense within an (optimitic) transaction?

For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.

Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.

I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.

On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:

> Hello all,
> some team members had a meeting yesterday, one of the discussed
> subjects was about using atomic operations (putIfAbsent, etc..).
> Mircea just summarised it in the following proposal:
>
> The atomic operations, as defined by the ConcurrentHashMap, don't fit
> well within the scope of optimistic transaction: this is because there
> is a discrepancy between the value returned by the operation and the
> value and the fact that the operation is applied or not:
> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
> the scope of the current transaction, but in fact there might be a
> value committed by another transaction, hidden by the fact we're
> running in repeatable read mode.
> Later on, at prepare time when the same operation is applied on the
> node that actually holds k, it might not succeed as another
> transaction has updated k in between, but the return value of the
> method was already evaluated long before this point.
> In order to solve this problem, if an atomic operations happens within
> the scope of a transaction, Infinispan eagerly acquires a lock on the
> remote node. This locks is held for the entire duration of the
> transaction, and is an expensive lock as it involves an RPC. If
> keeping the lock remotely for potentially long time represents a
> problem, the user can suspend the running transaction and run the
> atomic operation out of transaction's scope, then resume the
> transaction.
>
>
> In addition to this, would would you think about adding a flag to
> these methods which acts as suspending the transaction just before and
> resuming it right after? I don't know what is the cost of suspending &
> resuming a transaction, but such a flag could optionally be optimized
> in future by just ignoring the current transaction instead of really
> suspending it, or apply other clever tricks we might come across.
>
> I also think that we should discuss if such a behaviour should not be
> the default - anybody using an atomic operation is going to make some
> assumptions which are clearly incompatible with the transaction, so
> I'm wondering what is the path here to "least surprise" for default
> invocation.
>
> Regards,
> Sanne
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
I agree they don't make sense, but only in the sense of exposed API
during a transaction: some time ago I admit I was expecting them to
just work: the API is there, nice public methods in the public
interface with javadocs explaining that that was exactly what I was
looking for, no warnings, no failures. Even worse, all works fine when
running a local test because how the locks currently work they are
acquired locally first, so unless you're running such a test in DIST
mode, and happen to be *not* the owner of the being tested key, people
won't even notice that this is not supported.

Still being able to use them is very important, also in combination
with transactions: I might be running blocks of transactional code
(like a CRUD operation via OGM) and still require to advance a
sequence for primary key generation. This needs to be an atomic
operation, and I should really not forget to suspend the transaction.

Sanne

2011/7/4 Galder Zamarreño <[hidden email]>:

> Do these atomic operations really make sense within an (optimitic) transaction?
>
> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>
> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>
> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>
> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>
>> Hello all,
>> some team members had a meeting yesterday, one of the discussed
>> subjects was about using atomic operations (putIfAbsent, etc..).
>> Mircea just summarised it in the following proposal:
>>
>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>> well within the scope of optimistic transaction: this is because there
>> is a discrepancy between the value returned by the operation and the
>> value and the fact that the operation is applied or not:
>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>> the scope of the current transaction, but in fact there might be a
>> value committed by another transaction, hidden by the fact we're
>> running in repeatable read mode.
>> Later on, at prepare time when the same operation is applied on the
>> node that actually holds k, it might not succeed as another
>> transaction has updated k in between, but the return value of the
>> method was already evaluated long before this point.
>> In order to solve this problem, if an atomic operations happens within
>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>> remote node. This locks is held for the entire duration of the
>> transaction, and is an expensive lock as it involves an RPC. If
>> keeping the lock remotely for potentially long time represents a
>> problem, the user can suspend the running transaction and run the
>> atomic operation out of transaction's scope, then resume the
>> transaction.
>>
>>
>> In addition to this, would would you think about adding a flag to
>> these methods which acts as suspending the transaction just before and
>> resuming it right after? I don't know what is the cost of suspending &
>> resuming a transaction, but such a flag could optionally be optimized
>> in future by just ignoring the current transaction instead of really
>> suspending it, or apply other clever tricks we might come across.
>>
>> I also think that we should discuss if such a behaviour should not be
>> the default - anybody using an atomic operation is going to make some
>> assumptions which are clearly incompatible with the transaction, so
>> I'm wondering what is the path here to "least surprise" for default
>> invocation.
>>
>> Regards,
>> Sanne
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> --
> Galder Zamarreño
> Sr. Software Engineer
> Infinispan, JBoss Cache
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Galder Zamarreno


On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:

> I agree they don't make sense, but only in the sense of exposed API
> during a transaction: some time ago I admit I was expecting them to
> just work: the API is there, nice public methods in the public
> interface with javadocs explaining that that was exactly what I was
> looking for, no warnings, no failures. Even worse, all works fine when
> running a local test because how the locks currently work they are
> acquired locally first, so unless you're running such a test in DIST
> mode, and happen to be *not* the owner of the being tested key, people
> won't even notice that this is not supported.
>
> Still being able to use them is very important, also in combination
> with transactions: I might be running blocks of transactional code
> (like a CRUD operation via OGM) and still require to advance a
> sequence for primary key generation. This needs to be an atomic
> operation, and I should really not forget to suspend the transaction.

Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.

I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.

I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.

For example:

Cache contains: k1=galder

1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
3. Tx2 commits
4. Tx1 commits
End result: k1=sanne

1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
3. Tx2 rollback -> times out acquiring lock
4. Tx1 commits -> applies change
End result: k1=sanne

>
> Sanne
>
> 2011/7/4 Galder Zamarreño <[hidden email]>:
>> Do these atomic operations really make sense within an (optimitic) transaction?
>>
>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>
>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>
>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>
>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>
>>> Hello all,
>>> some team members had a meeting yesterday, one of the discussed
>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>> Mircea just summarised it in the following proposal:
>>>
>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>> well within the scope of optimistic transaction: this is because there
>>> is a discrepancy between the value returned by the operation and the
>>> value and the fact that the operation is applied or not:
>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>> the scope of the current transaction, but in fact there might be a
>>> value committed by another transaction, hidden by the fact we're
>>> running in repeatable read mode.
>>> Later on, at prepare time when the same operation is applied on the
>>> node that actually holds k, it might not succeed as another
>>> transaction has updated k in between, but the return value of the
>>> method was already evaluated long before this point.
>>> In order to solve this problem, if an atomic operations happens within
>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>> remote node. This locks is held for the entire duration of the
>>> transaction, and is an expensive lock as it involves an RPC. If
>>> keeping the lock remotely for potentially long time represents a
>>> problem, the user can suspend the running transaction and run the
>>> atomic operation out of transaction's scope, then resume the
>>> transaction.
>>>
>>>
>>> In addition to this, would would you think about adding a flag to
>>> these methods which acts as suspending the transaction just before and
>>> resuming it right after? I don't know what is the cost of suspending &
>>> resuming a transaction, but such a flag could optionally be optimized
>>> in future by just ignoring the current transaction instead of really
>>> suspending it, or apply other clever tricks we might come across.
>>>
>>> I also think that we should discuss if such a behaviour should not be
>>> the default - anybody using an atomic operation is going to make some
>>> assumptions which are clearly incompatible with the transaction, so
>>> I'm wondering what is the path here to "least surprise" for default
>>> invocation.
>>>
>>> Regards,
>>> Sanne
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>> Galder Zamarreño
>> Sr. Software Engineer
>> Infinispan, JBoss Cache
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Dan Berindei
On Tue, Jul 5, 2011 at 12:23 PM, Galder Zamarreño <[hidden email]> wrote:

>
>
> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>
>> I agree they don't make sense, but only in the sense of exposed API
>> during a transaction: some time ago I admit I was expecting them to
>> just work: the API is there, nice public methods in the public
>> interface with javadocs explaining that that was exactly what I was
>> looking for, no warnings, no failures. Even worse, all works fine when
>> running a local test because how the locks currently work they are
>> acquired locally first, so unless you're running such a test in DIST
>> mode, and happen to be *not* the owner of the being tested key, people
>> won't even notice that this is not supported.
>>
>> Still being able to use them is very important, also in combination
>> with transactions: I might be running blocks of transactional code
>> (like a CRUD operation via OGM) and still require to advance a
>> sequence for primary key generation. This needs to be an atomic
>> operation, and I should really not forget to suspend the transaction.
>
> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>
> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>
> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>
> For example:
>
> Cache contains: k1=galder
>
> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
> 3. Tx2 commits
> 4. Tx1 commits
> End result: k1=sanne
>
> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
> 3. Tx2 rollback -> times out acquiring lock
> 4. Tx1 commits -> applies change
> End result: k1=sanne
>

Galder, the big difference would be that with optimistic transactions
you don't acquire the lock on the spot, so the tx will be rolled back
if someone else modifies the key between your get and the prepare.
We could make a compromise, instead of forcing the atomic operations
to happen outside the transaction we could force them to always use
pessimistic locking. Actually Mircea and I discussed this Friday
evening too, but I forgot about it until now.

After all Sanne has two use cases for atomic operations: sequences and
reference counts. Sequences can and should happen outside
transactions, but as we discussed on the train we could make each
node/thread acquire a range of say 100 seq numbers at a time and
remove the need for any locking to get a new sequence number.
Reference counts on the other hand should remain inside the
transaction, because you would have to to the refcount rollback by
hand (plus I don't think you should let other transactions see the
modified refcount before they see the new data).

Dan

>>
>> Sanne
>>
>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>
>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>
>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>
>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>
>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>
>>>> Hello all,
>>>> some team members had a meeting yesterday, one of the discussed
>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>> Mircea just summarised it in the following proposal:
>>>>
>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>> well within the scope of optimistic transaction: this is because there
>>>> is a discrepancy between the value returned by the operation and the
>>>> value and the fact that the operation is applied or not:
>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>> the scope of the current transaction, but in fact there might be a
>>>> value committed by another transaction, hidden by the fact we're
>>>> running in repeatable read mode.
>>>> Later on, at prepare time when the same operation is applied on the
>>>> node that actually holds k, it might not succeed as another
>>>> transaction has updated k in between, but the return value of the
>>>> method was already evaluated long before this point.
>>>> In order to solve this problem, if an atomic operations happens within
>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>> remote node. This locks is held for the entire duration of the
>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>> keeping the lock remotely for potentially long time represents a
>>>> problem, the user can suspend the running transaction and run the
>>>> atomic operation out of transaction's scope, then resume the
>>>> transaction.
>>>>
>>>>
>>>> In addition to this, would would you think about adding a flag to
>>>> these methods which acts as suspending the transaction just before and
>>>> resuming it right after? I don't know what is the cost of suspending &
>>>> resuming a transaction, but such a flag could optionally be optimized
>>>> in future by just ignoring the current transaction instead of really
>>>> suspending it, or apply other clever tricks we might come across.
>>>>
>>>> I also think that we should discuss if such a behaviour should not be
>>>> the default - anybody using an atomic operation is going to make some
>>>> assumptions which are clearly incompatible with the transaction, so
>>>> I'm wondering what is the path here to "least surprise" for default
>>>> invocation.
>>>>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
In reply to this post by Galder Zamarreno
2011/7/5 Galder Zamarreño <[hidden email]>:

>
>
> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>
>> I agree they don't make sense, but only in the sense of exposed API
>> during a transaction: some time ago I admit I was expecting them to
>> just work: the API is there, nice public methods in the public
>> interface with javadocs explaining that that was exactly what I was
>> looking for, no warnings, no failures. Even worse, all works fine when
>> running a local test because how the locks currently work they are
>> acquired locally first, so unless you're running such a test in DIST
>> mode, and happen to be *not* the owner of the being tested key, people
>> won't even notice that this is not supported.
>>
>> Still being able to use them is very important, also in combination
>> with transactions: I might be running blocks of transactional code
>> (like a CRUD operation via OGM) and still require to advance a
>> sequence for primary key generation. This needs to be an atomic
>> operation, and I should really not forget to suspend the transaction.
>
> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>
> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>
> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>
> For example:
>
> Cache contains: k1=galder
>
> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
> 3. Tx2 commits
> 4. Tx1 commits
> End result: k1=sanne

Right.
To clarify, this is what would happen with the current implementation:

1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
returned "galder"
2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
the scope of this transaction, but not seen by other tx
3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
assigned, as because of repeatable read we're still seeing "galder"
4. Tx2  & Tx1 commit

..and the end result depends on who commits first.

>
> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
> 3. Tx2 rollback -> times out acquiring lock
> 4. Tx1 commits -> applies change
> End result: k1=sanne

I'm not sure we're on the same line here. 1) should apply the
operation right away, so even if it might very briefly have to acquire
a lock on it, it's immediately released (not at the end of the
transaction), so why would TX2 have to wait for it to the point it
needs to rollback?


>
>>
>> Sanne
>>
>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>
>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>
>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>
>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>
>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>
>>>> Hello all,
>>>> some team members had a meeting yesterday, one of the discussed
>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>> Mircea just summarised it in the following proposal:
>>>>
>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>> well within the scope of optimistic transaction: this is because there
>>>> is a discrepancy between the value returned by the operation and the
>>>> value and the fact that the operation is applied or not:
>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>> the scope of the current transaction, but in fact there might be a
>>>> value committed by another transaction, hidden by the fact we're
>>>> running in repeatable read mode.
>>>> Later on, at prepare time when the same operation is applied on the
>>>> node that actually holds k, it might not succeed as another
>>>> transaction has updated k in between, but the return value of the
>>>> method was already evaluated long before this point.
>>>> In order to solve this problem, if an atomic operations happens within
>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>> remote node. This locks is held for the entire duration of the
>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>> keeping the lock remotely for potentially long time represents a
>>>> problem, the user can suspend the running transaction and run the
>>>> atomic operation out of transaction's scope, then resume the
>>>> transaction.
>>>>
>>>>
>>>> In addition to this, would would you think about adding a flag to
>>>> these methods which acts as suspending the transaction just before and
>>>> resuming it right after? I don't know what is the cost of suspending &
>>>> resuming a transaction, but such a flag could optionally be optimized
>>>> in future by just ignoring the current transaction instead of really
>>>> suspending it, or apply other clever tricks we might come across.
>>>>
>>>> I also think that we should discuss if such a behaviour should not be
>>>> the default - anybody using an atomic operation is going to make some
>>>> assumptions which are clearly incompatible with the transaction, so
>>>> I'm wondering what is the path here to "least surprise" for default
>>>> invocation.
>>>>
>>>> Regards,
>>>> Sanne
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> --
>>> Galder Zamarreño
>>> Sr. Software Engineer
>>> Infinispan, JBoss Cache
>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> --
> Galder Zamarreño
> Sr. Software Engineer
> Infinispan, JBoss Cache
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
In reply to this post by Dan Berindei
2011/7/5 Dan Berindei <[hidden email]>:

> On Tue, Jul 5, 2011 at 12:23 PM, Galder Zamarreño <[hidden email]> wrote:
>>
>>
>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>
>>> I agree they don't make sense, but only in the sense of exposed API
>>> during a transaction: some time ago I admit I was expecting them to
>>> just work: the API is there, nice public methods in the public
>>> interface with javadocs explaining that that was exactly what I was
>>> looking for, no warnings, no failures. Even worse, all works fine when
>>> running a local test because how the locks currently work they are
>>> acquired locally first, so unless you're running such a test in DIST
>>> mode, and happen to be *not* the owner of the being tested key, people
>>> won't even notice that this is not supported.
>>>
>>> Still being able to use them is very important, also in combination
>>> with transactions: I might be running blocks of transactional code
>>> (like a CRUD operation via OGM) and still require to advance a
>>> sequence for primary key generation. This needs to be an atomic
>>> operation, and I should really not forget to suspend the transaction.
>>
>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>
>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>
>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>
>> For example:
>>
>> Cache contains: k1=galder
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>> 3. Tx2 commits
>> 4. Tx1 commits
>> End result: k1=sanne
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>> 3. Tx2 rollback -> times out acquiring lock
>> 4. Tx1 commits -> applies change
>> End result: k1=sanne
>>
>
> Galder, the big difference would be that with optimistic transactions
> you don't acquire the lock on the spot, so the tx will be rolled back
> if someone else modifies the key between your get and the prepare.
> We could make a compromise, instead of forcing the atomic operations
> to happen outside the transaction we could force them to always use
> pessimistic locking. Actually Mircea and I discussed this Friday
> evening too, but I forgot about it until now.
>
> After all Sanne has two use cases for atomic operations: sequences and
> reference counts. Sequences can and should happen outside
> transactions, but as we discussed on the train we could make each
> node/thread acquire a range of say 100 seq numbers at a time and
> remove the need for any locking to get a new sequence number.
> Reference counts on the other hand should remain inside the
> transaction, because you would have to to the refcount rollback by
> hand (plus I don't think you should let other transactions see the
> modified refcount before they see the new data).

To clarify my use case, I'm never going to need refcounts together to
a running transaction. The refcount example was about why I need
atomic counters, but this thread is about the dangers of exposing
these atomic operations API which are broken when using transactions.

Sanne

>
> Dan
>
>>>
>>> Sanne
>>>
>>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>>
>>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>>
>>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>>
>>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>>
>>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>>
>>>>> Hello all,
>>>>> some team members had a meeting yesterday, one of the discussed
>>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>>> Mircea just summarised it in the following proposal:
>>>>>
>>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>>> well within the scope of optimistic transaction: this is because there
>>>>> is a discrepancy between the value returned by the operation and the
>>>>> value and the fact that the operation is applied or not:
>>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>>> the scope of the current transaction, but in fact there might be a
>>>>> value committed by another transaction, hidden by the fact we're
>>>>> running in repeatable read mode.
>>>>> Later on, at prepare time when the same operation is applied on the
>>>>> node that actually holds k, it might not succeed as another
>>>>> transaction has updated k in between, but the return value of the
>>>>> method was already evaluated long before this point.
>>>>> In order to solve this problem, if an atomic operations happens within
>>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>>> remote node. This locks is held for the entire duration of the
>>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>>> keeping the lock remotely for potentially long time represents a
>>>>> problem, the user can suspend the running transaction and run the
>>>>> atomic operation out of transaction's scope, then resume the
>>>>> transaction.
>>>>>
>>>>>
>>>>> In addition to this, would would you think about adding a flag to
>>>>> these methods which acts as suspending the transaction just before and
>>>>> resuming it right after? I don't know what is the cost of suspending &
>>>>> resuming a transaction, but such a flag could optionally be optimized
>>>>> in future by just ignoring the current transaction instead of really
>>>>> suspending it, or apply other clever tricks we might come across.
>>>>>
>>>>> I also think that we should discuss if such a behaviour should not be
>>>>> the default - anybody using an atomic operation is going to make some
>>>>> assumptions which are clearly incompatible with the transaction, so
>>>>> I'm wondering what is the path here to "least surprise" for default
>>>>> invocation.
>>>>>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Dan Berindei
On Tue, Jul 5, 2011 at 12:49 PM, Sanne Grinovero <[hidden email]> wrote:

>
> 2011/7/5 Dan Berindei <[hidden email]>:
>>
>> After all Sanne has two use cases for atomic operations: sequences and
>> reference counts. Sequences can and should happen outside
>> transactions, but as we discussed on the train we could make each
>> node/thread acquire a range of say 100 seq numbers at a time and
>> remove the need for any locking to get a new sequence number.
>> Reference counts on the other hand should remain inside the
>> transaction, because you would have to to the refcount rollback by
>> hand (plus I don't think you should let other transactions see the
>> modified refcount before they see the new data).
>
> To clarify my use case, I'm never going to need refcounts together to
> a running transaction. The refcount example was about why I need
> atomic counters, but this thread is about the dangers of exposing
> these atomic operations API which are broken when using transactions.
>

O, I understand refcounts are not really the topic you wanted to talk
about, but I'd really like to know why you don't need transactions
with refcounts. I was assuming the refcount is a link from a key A in
the cache to another key B, so adding another link would involve
incrementing the counter as well as adding the key A.

Or do you mean that refcount increment/decrement should be a special
operation that never fails, so it could be done after the commit of
the tx that added key A?
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Dan Berindei
In reply to this post by Sanne Grinovero-3
On Tue, Jul 5, 2011 at 12:46 PM, Sanne Grinovero <[hidden email]> wrote:

> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>
>>
>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>
>>> I agree they don't make sense, but only in the sense of exposed API
>>> during a transaction: some time ago I admit I was expecting them to
>>> just work: the API is there, nice public methods in the public
>>> interface with javadocs explaining that that was exactly what I was
>>> looking for, no warnings, no failures. Even worse, all works fine when
>>> running a local test because how the locks currently work they are
>>> acquired locally first, so unless you're running such a test in DIST
>>> mode, and happen to be *not* the owner of the being tested key, people
>>> won't even notice that this is not supported.
>>>
>>> Still being able to use them is very important, also in combination
>>> with transactions: I might be running blocks of transactional code
>>> (like a CRUD operation via OGM) and still require to advance a
>>> sequence for primary key generation. This needs to be an atomic
>>> operation, and I should really not forget to suspend the transaction.
>>
>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>
>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>
>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>
>> For example:
>>
>> Cache contains: k1=galder
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>> 3. Tx2 commits
>> 4. Tx1 commits
>> End result: k1=sanne
>
> Right.
> To clarify, this is what would happen with the current implementation:
>
> 1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
> returned "galder"
> 2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
> the scope of this transaction, but not seen by other tx
> 3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
> assigned, as because of repeatable read we're still seeing "galder"
> 4. Tx2  & Tx1 commit
>
> ..and the end result depends on who commits first.
>
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>> 3. Tx2 rollback -> times out acquiring lock
>> 4. Tx1 commits -> applies change
>> End result: k1=sanne
>
> I'm not sure we're on the same line here. 1) should apply the
> operation right away, so even if it might very briefly have to acquire
> a lock on it, it's immediately released (not at the end of the
> transaction), so why would TX2 have to wait for it to the point it
> needs to rollback?
>

I think it would make sense to make atomic operations pessimistic by
default, so they would behave like in Galder's example.

Then if you wanted to reduce contention you could suspend/resume the
transaction around your atomic operations and make them behave like
you're expecting them to.

Here is a contrived example:

1. Start tx Tx1
2. cache.get("k") -> "v0"
3. cache.replace("k", "v0", "v1")
4. gache.get("k") -> ??

With repeatable read and suspend/resume around atomic operations, I
believe operation 4 would return "v0", and that would be very
surprising for a new user.
So I'd rather require explicit suspend/resume calls to make sure
anyone who uses atomic operations in a transaction understands what
results he's going to get.

Dan

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
2011/7/5 Dan Berindei <[hidden email]>:

> On Tue, Jul 5, 2011 at 12:46 PM, Sanne Grinovero <[hidden email]> wrote:
>> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>>
>>>
>>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>>
>>>> I agree they don't make sense, but only in the sense of exposed API
>>>> during a transaction: some time ago I admit I was expecting them to
>>>> just work: the API is there, nice public methods in the public
>>>> interface with javadocs explaining that that was exactly what I was
>>>> looking for, no warnings, no failures. Even worse, all works fine when
>>>> running a local test because how the locks currently work they are
>>>> acquired locally first, so unless you're running such a test in DIST
>>>> mode, and happen to be *not* the owner of the being tested key, people
>>>> won't even notice that this is not supported.
>>>>
>>>> Still being able to use them is very important, also in combination
>>>> with transactions: I might be running blocks of transactional code
>>>> (like a CRUD operation via OGM) and still require to advance a
>>>> sequence for primary key generation. This needs to be an atomic
>>>> operation, and I should really not forget to suspend the transaction.
>>>
>>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>>
>>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>>
>>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>>
>>> For example:
>>>
>>> Cache contains: k1=galder
>>>
>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>>> 3. Tx2 commits
>>> 4. Tx1 commits
>>> End result: k1=sanne
>>
>> Right.
>> To clarify, this is what would happen with the current implementation:
>>
>> 1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
>> returned "galder"
>> 2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
>> the scope of this transaction, but not seen by other tx
>> 3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
>> assigned, as because of repeatable read we're still seeing "galder"
>> 4. Tx2  & Tx1 commit
>>
>> ..and the end result depends on who commits first.
>>
>>>
>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>>> 3. Tx2 rollback -> times out acquiring lock
>>> 4. Tx1 commits -> applies change
>>> End result: k1=sanne
>>
>> I'm not sure we're on the same line here. 1) should apply the
>> operation right away, so even if it might very briefly have to acquire
>> a lock on it, it's immediately released (not at the end of the
>> transaction), so why would TX2 have to wait for it to the point it
>> needs to rollback?
>>
>
> I think it would make sense to make atomic operations pessimistic by
> default, so they would behave like in Galder's example.
>
> Then if you wanted to reduce contention you could suspend/resume the
> transaction around your atomic operations and make them behave like
> you're expecting them to.
>
> Here is a contrived example:
>
> 1. Start tx Tx1
> 2. cache.get("k") -> "v0"
> 3. cache.replace("k", "v0", "v1")
> 4. gache.get("k") -> ??
>
> With repeatable read and suspend/resume around atomic operations, I
> believe operation 4 would return "v0", and that would be very
> surprising for a new user.
> So I'd rather require explicit suspend/resume calls to make sure
> anyone who uses atomic operations in a transaction understands what
> results he's going to get.

The problem is that as a use case it makes no sense to use an atomic
operation without evaluating the return value.
so 3) should actually read like

3. boolean done = cache.replace("k", "v0", "v1")
and based on this value, the application would branch in some way, and
so acquiring locks and waiting for each other is not enough, we can
only support this if write skew checks are enabled, and mandate the
full operation to rollback in the end. That might be one option, but I
really don't like to make it likely to rollback transactions, I'd
prefer to have an alternative like a new flag which enforces a "fresh
read" skipping the repeatable read guarantees. Of course this wouldn't
work if we're not actually sending the operations to the key owners,
so suspending the transaction is a much nicer approach from the user
perspective. Though I agree this behaviour should be selectable.

Cheers,
Sanne

>
> Dan
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
In reply to this post by Dan Berindei
2011/7/5 Dan Berindei <[hidden email]>:

> On Tue, Jul 5, 2011 at 12:46 PM, Sanne Grinovero <[hidden email]> wrote:
>> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>>
>>>
>>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>>
>>>> I agree they don't make sense, but only in the sense of exposed API
>>>> during a transaction: some time ago I admit I was expecting them to
>>>> just work: the API is there, nice public methods in the public
>>>> interface with javadocs explaining that that was exactly what I was
>>>> looking for, no warnings, no failures. Even worse, all works fine when
>>>> running a local test because how the locks currently work they are
>>>> acquired locally first, so unless you're running such a test in DIST
>>>> mode, and happen to be *not* the owner of the being tested key, people
>>>> won't even notice that this is not supported.
>>>>
>>>> Still being able to use them is very important, also in combination
>>>> with transactions: I might be running blocks of transactional code
>>>> (like a CRUD operation via OGM) and still require to advance a
>>>> sequence for primary key generation. This needs to be an atomic
>>>> operation, and I should really not forget to suspend the transaction.
>>>
>>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>>
>>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>>
>>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>>
>>> For example:
>>>
>>> Cache contains: k1=galder
>>>
>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>>> 3. Tx2 commits
>>> 4. Tx1 commits
>>> End result: k1=sanne
>>
>> Right.
>> To clarify, this is what would happen with the current implementation:
>>
>> 1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
>> returned "galder"
>> 2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
>> the scope of this transaction, but not seen by other tx
>> 3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
>> assigned, as because of repeatable read we're still seeing "galder"
>> 4. Tx2  & Tx1 commit
>>
>> ..and the end result depends on who commits first.
>>
>>>
>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>>> 3. Tx2 rollback -> times out acquiring lock
>>> 4. Tx1 commits -> applies change
>>> End result: k1=sanne
>>
>> I'm not sure we're on the same line here. 1) should apply the
>> operation right away, so even if it might very briefly have to acquire
>> a lock on it, it's immediately released (not at the end of the
>> transaction), so why would TX2 have to wait for it to the point it
>> needs to rollback?
>>
>
> I think it would make sense to make atomic operations pessimistic by
> default, so they would behave like in Galder's example.
>
> Then if you wanted to reduce contention you could suspend/resume the
> transaction around your atomic operations and make them behave like
> you're expecting them to.
>
> Here is a contrived example:
>
> 1. Start tx Tx1
> 2. cache.get("k") -> "v0"
> 3. cache.replace("k", "v0", "v1")
> 4. gache.get("k") -> ??
>
> With repeatable read and suspend/resume around atomic operations, I
> believe operation 4 would return "v0", and that would be very
> surprising for a new user.
> So I'd rather require explicit suspend/resume calls to make sure
> anyone who uses atomic operations in a transaction understands what
> results he's going to get.

That's an interesting case, but I wasn't thinking about that. So it
might become useful later on.
The refcount scenario I'd like to improve first is about garbage
collection of old unused index segments,
we're counting references from open searchers and allow to finish to
run queries on still open segments
while they are being deleted: at the same time, these delete
operations are run in batch operations in background threads,
I couldn't possibly run them in a transaction as it would likely not
have enough memory to complete it, and anyway they're run async
to the rest of the application. So timing is not very critical, but
having a wrong increment/decrement on the counter can cause
many issues and so this must rely on atomic operations. Current
implementation acquires a pessimistic lock on the integer,
having a distributed "AtomicInteger" as discusses on the train would
be a simple improvement.

Sanne

>
> Dan
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Dan Berindei
On Tue, Jul 5, 2011 at 1:45 PM, Sanne Grinovero <[hidden email]> wrote:

> That's an interesting case, but I wasn't thinking about that. So it
> might become useful later on.
> The refcount scenario I'd like to improve first is about garbage
> collection of old unused index segments,
> we're counting references from open searchers and allow to finish to
> run queries on still open segments
> while they are being deleted: at the same time, these delete
> operations are run in batch operations in background threads,
> I couldn't possibly run them in a transaction as it would likely not
> have enough memory to complete it, and anyway they're run async
> to the rest of the application. So timing is not very critical, but
> having a wrong increment/decrement on the counter can cause
> many issues and so this must rely on atomic operations. Current
> implementation acquires a pessimistic lock on the integer,
> having a distributed "AtomicInteger" as discusses on the train would
> be a simple improvement.
>

I think I got it now, your references are from application objects to
cache keys, so you don't have anything else that you need to modify in
the same transaction.
You only have reads, unless the refcount reaches 0, in which case you
also do a remove.

It's clear to me now that the distributed AtomicInteger approach would
be best in your case, so that means I haven't found a real-life
problem that requires in-transaction atomic operations yet ;-)

Dan


> Sanne
>
>>
>> Dan
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Dan Berindei
In reply to this post by Sanne Grinovero-3
On Tue, Jul 5, 2011 at 1:39 PM, Sanne Grinovero <[hidden email]> wrote:

> 2011/7/5 Dan Berindei <[hidden email]>:
>> Here is a contrived example:
>>
>> 1. Start tx Tx1
>> 2. cache.get("k") -> "v0"
>> 3. cache.replace("k", "v0", "v1")
>> 4. gache.get("k") -> ??
>>
>> With repeatable read and suspend/resume around atomic operations, I
>> believe operation 4 would return "v0", and that would be very
>> surprising for a new user.
>> So I'd rather require explicit suspend/resume calls to make sure
>> anyone who uses atomic operations in a transaction understands what
>> results he's going to get.
>
> The problem is that as a use case it makes no sense to use an atomic
> operation without evaluating the return value.
> so 3) should actually read like
>
> 3. boolean done = cache.replace("k", "v0", "v1")
> and based on this value, the application would branch in some way, and
> so acquiring locks and waiting for each other is not enough, we can
> only support this if write skew checks are enabled, and mandate the
> full operation to rollback in the end. That might be one option, but I
> really don't like to make it likely to rollback transactions, I'd
> prefer to have an alternative like a new flag which enforces a "fresh
> read" skipping the repeatable read guarantees. Of course this wouldn't
> work if we're not actually sending the operations to the key owners,
> so suspending the transaction is a much nicer approach from the user
> perspective. Though I agree this behaviour should be selectable.
>

Ok, I'm slowly remembering your arguments... do you think the "fresh
read" flag should be available for all operations, or does it make
sense to make it an internal flag that only the atomic operations will
use?

To summarize, with this example:
1. Start tx
2. cache.get("k") -> "v0"
3. cache.replace("k", "v0", "v1")
4. gache.get("k") -> ??
5. Commit tx

The options we could support are:
a. Tx suspend: no retries, but also no rollback for replace() and 4)
will not see the updated value
b. Optimistic locking + write skew check: if the key is modified by
another tx between 2) and 5), the entire transaction has to be redone
c. Optimistic locking + write skew check + fresh read: we only have to
redo the tx if the key is modified by another tx between 3) and 5)
d. Pessimistic locking: if the key is modified between 2) and 5), the
entire transaction has to be redone
e. Pessimistic locking + fresh read: no redo, but decreased throughput
because we hold the lock between 3) and 5)

I guess there is no reason to support option d), as we're making an
RPC to the owner in order to get the lock anyway. I think I'm leaning
towards supporting only a) and e), but there might be cases where b)
and c) would perform better.

Dan
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Galder Zamarreno
In reply to this post by Dan Berindei

On Jul 5, 2011, at 11:45 AM, Dan Berindei wrote:

> On Tue, Jul 5, 2011 at 12:23 PM, Galder Zamarreño <[hidden email]> wrote:
>>
>>
>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>
>>> I agree they don't make sense, but only in the sense of exposed API
>>> during a transaction: some time ago I admit I was expecting them to
>>> just work: the API is there, nice public methods in the public
>>> interface with javadocs explaining that that was exactly what I was
>>> looking for, no warnings, no failures. Even worse, all works fine when
>>> running a local test because how the locks currently work they are
>>> acquired locally first, so unless you're running such a test in DIST
>>> mode, and happen to be *not* the owner of the being tested key, people
>>> won't even notice that this is not supported.
>>>
>>> Still being able to use them is very important, also in combination
>>> with transactions: I might be running blocks of transactional code
>>> (like a CRUD operation via OGM) and still require to advance a
>>> sequence for primary key generation. This needs to be an atomic
>>> operation, and I should really not forget to suspend the transaction.
>>
>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>
>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>
>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>
>> For example:
>>
>> Cache contains: k1=galder
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>> 3. Tx2 commits
>> 4. Tx1 commits
>> End result: k1=sanne
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>> 3. Tx2 rollback -> times out acquiring lock
>> 4. Tx1 commits -> applies change
>> End result: k1=sanne
>>
>
> Galder, the big difference would be that with optimistic transactions
> you don't acquire the lock on the spot, so the tx will be rolled back
> if someone else modifies the key between your get and the prepare.

Just to clarify, the 2nd set of operations are not mean to represent optimistic transactions, but rather the current locking strategy.

That's what important for the current Infinispan users, what will behave in a different way to what they've been used to throughout the 4.x series.

> We could make a compromise, instead of forcing the atomic operations
> to happen outside the transaction we could force them to always use
> pessimistic locking. Actually Mircea and I discussed this Friday
> evening too, but I forgot about it until now.
>
> After all Sanne has two use cases for atomic operations: sequences and
> reference counts. Sequences can and should happen outside
> transactions, but as we discussed on the train we could make each
> node/thread acquire a range of say 100 seq numbers at a time and
> remove the need for any locking to get a new sequence number.
> Reference counts on the other hand should remain inside the
> transaction, because you would have to to the refcount rollback by
> hand (plus I don't think you should let other transactions see the
> modified refcount before they see the new data).
>
> Dan
>
>>>
>>> Sanne
>>>
>>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>>
>>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>>
>>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>>
>>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>>
>>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>>
>>>>> Hello all,
>>>>> some team members had a meeting yesterday, one of the discussed
>>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>>> Mircea just summarised it in the following proposal:
>>>>>
>>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>>> well within the scope of optimistic transaction: this is because there
>>>>> is a discrepancy between the value returned by the operation and the
>>>>> value and the fact that the operation is applied or not:
>>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>>> the scope of the current transaction, but in fact there might be a
>>>>> value committed by another transaction, hidden by the fact we're
>>>>> running in repeatable read mode.
>>>>> Later on, at prepare time when the same operation is applied on the
>>>>> node that actually holds k, it might not succeed as another
>>>>> transaction has updated k in between, but the return value of the
>>>>> method was already evaluated long before this point.
>>>>> In order to solve this problem, if an atomic operations happens within
>>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>>> remote node. This locks is held for the entire duration of the
>>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>>> keeping the lock remotely for potentially long time represents a
>>>>> problem, the user can suspend the running transaction and run the
>>>>> atomic operation out of transaction's scope, then resume the
>>>>> transaction.
>>>>>
>>>>>
>>>>> In addition to this, would would you think about adding a flag to
>>>>> these methods which acts as suspending the transaction just before and
>>>>> resuming it right after? I don't know what is the cost of suspending &
>>>>> resuming a transaction, but such a flag could optionally be optimized
>>>>> in future by just ignoring the current transaction instead of really
>>>>> suspending it, or apply other clever tricks we might come across.
>>>>>
>>>>> I also think that we should discuss if such a behaviour should not be
>>>>> the default - anybody using an atomic operation is going to make some
>>>>> assumptions which are clearly incompatible with the transaction, so
>>>>> I'm wondering what is the path here to "least surprise" for default
>>>>> invocation.
>>>>>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Galder Zamarreno
In reply to this post by Sanne Grinovero-3

On Jul 5, 2011, at 11:46 AM, Sanne Grinovero wrote:

> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>
>>
>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>
>>> I agree they don't make sense, but only in the sense of exposed API
>>> during a transaction: some time ago I admit I was expecting them to
>>> just work: the API is there, nice public methods in the public
>>> interface with javadocs explaining that that was exactly what I was
>>> looking for, no warnings, no failures. Even worse, all works fine when
>>> running a local test because how the locks currently work they are
>>> acquired locally first, so unless you're running such a test in DIST
>>> mode, and happen to be *not* the owner of the being tested key, people
>>> won't even notice that this is not supported.
>>>
>>> Still being able to use them is very important, also in combination
>>> with transactions: I might be running blocks of transactional code
>>> (like a CRUD operation via OGM) and still require to advance a
>>> sequence for primary key generation. This needs to be an atomic
>>> operation, and I should really not forget to suspend the transaction.
>>
>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>
>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>
>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>
>> For example:
>>
>> Cache contains: k1=galder
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>> 3. Tx2 commits
>> 4. Tx1 commits
>> End result: k1=sanne
>
> Right.
> To clarify, this is what would happen with the current implementation:
>
> 1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
> returned "galder"
> 2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
> the scope of this transaction, but not seen by other tx
> 3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
> assigned, as because of repeatable read we're still seeing "galder"
> 4. Tx2  & Tx1 commit
>
> ..and the end result depends on who commits first.

The sequence of events above is what I suppose would happen with the suspended tx mode, not the current impl

>
>>
>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>> 3. Tx2 rollback -> times out acquiring lock
>> 4. Tx1 commits -> applies change
>> End result: k1=sanne
>
> I'm not sure we're on the same line here. 1) should apply the
> operation right away, so even if it might very briefly have to acquire
> a lock on it, it's immediately released (not at the end of the
> transaction), so why would TX2 have to wait for it to the point it
> needs to rollback?

This is what I was trying to picture as current implementation. It's true that it should apply the operation, but it also acquires the lock, at least in local mode and the locks are only release at prepare/commit time.

Well, tx2 is trying to acquire a WL on a entry that's being modified by TX1. Here I'm assuming that Tx1 does 'something else' and so Tx2 times out waiting for the lock.

>
>
>>
>>>
>>> Sanne
>>>
>>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>>
>>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>>
>>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>>
>>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>>
>>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>>
>>>>> Hello all,
>>>>> some team members had a meeting yesterday, one of the discussed
>>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>>> Mircea just summarised it in the following proposal:
>>>>>
>>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>>> well within the scope of optimistic transaction: this is because there
>>>>> is a discrepancy between the value returned by the operation and the
>>>>> value and the fact that the operation is applied or not:
>>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>>> the scope of the current transaction, but in fact there might be a
>>>>> value committed by another transaction, hidden by the fact we're
>>>>> running in repeatable read mode.
>>>>> Later on, at prepare time when the same operation is applied on the
>>>>> node that actually holds k, it might not succeed as another
>>>>> transaction has updated k in between, but the return value of the
>>>>> method was already evaluated long before this point.
>>>>> In order to solve this problem, if an atomic operations happens within
>>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>>> remote node. This locks is held for the entire duration of the
>>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>>> keeping the lock remotely for potentially long time represents a
>>>>> problem, the user can suspend the running transaction and run the
>>>>> atomic operation out of transaction's scope, then resume the
>>>>> transaction.
>>>>>
>>>>>
>>>>> In addition to this, would would you think about adding a flag to
>>>>> these methods which acts as suspending the transaction just before and
>>>>> resuming it right after? I don't know what is the cost of suspending &
>>>>> resuming a transaction, but such a flag could optionally be optimized
>>>>> in future by just ignoring the current transaction instead of really
>>>>> suspending it, or apply other clever tricks we might come across.
>>>>>
>>>>> I also think that we should discuss if such a behaviour should not be
>>>>> the default - anybody using an atomic operation is going to make some
>>>>> assumptions which are clearly incompatible with the transaction, so
>>>>> I'm wondering what is the path here to "least surprise" for default
>>>>> invocation.
>>>>>
>>>>> Regards,
>>>>> Sanne
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> [hidden email]
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>> --
>>>> Galder Zamarreño
>>>> Sr. Software Engineer
>>>> Infinispan, JBoss Cache
>>>>
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>> Galder Zamarreño
>> Sr. Software Engineer
>> Infinispan, JBoss Cache
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
In reply to this post by Dan Berindei
2011/7/5 Dan Berindei <[hidden email]>:

> On Tue, Jul 5, 2011 at 1:39 PM, Sanne Grinovero <[hidden email]> wrote:
>> 2011/7/5 Dan Berindei <[hidden email]>:
>>> Here is a contrived example:
>>>
>>> 1. Start tx Tx1
>>> 2. cache.get("k") -> "v0"
>>> 3. cache.replace("k", "v0", "v1")
>>> 4. gache.get("k") -> ??
>>>
>>> With repeatable read and suspend/resume around atomic operations, I
>>> believe operation 4 would return "v0", and that would be very
>>> surprising for a new user.
>>> So I'd rather require explicit suspend/resume calls to make sure
>>> anyone who uses atomic operations in a transaction understands what
>>> results he's going to get.
>>
>> The problem is that as a use case it makes no sense to use an atomic
>> operation without evaluating the return value.
>> so 3) should actually read like
>>
>> 3. boolean done = cache.replace("k", "v0", "v1")
>> and based on this value, the application would branch in some way, and
>> so acquiring locks and waiting for each other is not enough, we can
>> only support this if write skew checks are enabled, and mandate the
>> full operation to rollback in the end. That might be one option, but I
>> really don't like to make it likely to rollback transactions, I'd
>> prefer to have an alternative like a new flag which enforces a "fresh
>> read" skipping the repeatable read guarantees. Of course this wouldn't
>> work if we're not actually sending the operations to the key owners,
>> so suspending the transaction is a much nicer approach from the user
>> perspective. Though I agree this behaviour should be selectable.
>>
>
> Ok, I'm slowly remembering your arguments... do you think the "fresh
> read" flag should be available for all operations, or does it make
> sense to make it an internal flag that only the atomic operations will
> use?
>
> To summarize, with this example:
> 1. Start tx
> 2. cache.get("k") -> "v0"
> 3. cache.replace("k", "v0", "v1")
> 4. gache.get("k") -> ??
> 5. Commit tx
>
> The options we could support are:
> a. Tx suspend: no retries, but also no rollback for replace() and 4)
> will not see the updated value

might work, but looks like a crazy user experience.

> b. Optimistic locking + write skew check: if the key is modified by
> another tx between 2) and 5), the entire transaction has to be redone

might work as well, since people opted in for "optimistic" they should
be prepared to experience failures.
I'm not sure what the "+" stands for, how can you have optimistic
locking without write skew checks?

> c. Optimistic locking + write skew check + fresh read: we only have to
> redo the tx if the key is modified by another tx between 3) and 5)

in this case we're breaking the repeatable read guarantees, so we
should clarify this very well.

> d. Pessimistic locking: if the key is modified between 2) and 5), the
> entire transaction has to be redone

I don't understand what's pessimistic about this? To be pessimistic it
would attempt to guarantee success by locking at 2): during the get
operation, before returning the value.
Also "if they key is modified" implies write skew checks, so how would
this be different than previous proposals?
Generally as a user if I'm opting in for a pessimistic lock the only
exception I'm prepared to handle is a timeout, definitely not a "try
again, the values changed".

> e. Pessimistic locking + fresh read: no redo, but decreased throughput
> because we hold the lock between 3) and 5)

I assume you really mean to do explicit pessimistic locking:
1. Start tx
2. cache.lock("k");
3. cache.get("k") -> "v0"
4. cache.replace("k", "v0", "v1") /// -> throw an exception if we're
not owning the lock
5. gache.get("k") -> ??
6. Commit tx

> I guess there is no reason to support option d), as we're making an
> RPC to the owner in order to get the lock anyway. I think I'm leaning
> towards supporting only a) and e), but there might be cases where b)
> and c) would perform better.
>
> Dan
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Galder Zamarreno
In reply to this post by Dan Berindei

On Jul 5, 2011, at 1:24 PM, Dan Berindei wrote:

> On Tue, Jul 5, 2011 at 1:39 PM, Sanne Grinovero <[hidden email]> wrote:
>> 2011/7/5 Dan Berindei <[hidden email]>:
>>> Here is a contrived example:
>>>
>>> 1. Start tx Tx1
>>> 2. cache.get("k") -> "v0"
>>> 3. cache.replace("k", "v0", "v1")
>>> 4. gache.get("k") -> ??
>>>
>>> With repeatable read and suspend/resume around atomic operations, I
>>> believe operation 4 would return "v0", and that would be very
>>> surprising for a new user.
>>> So I'd rather require explicit suspend/resume calls to make sure
>>> anyone who uses atomic operations in a transaction understands what
>>> results he's going to get.
>>
>> The problem is that as a use case it makes no sense to use an atomic
>> operation without evaluating the return value.
>> so 3) should actually read like
>>
>> 3. boolean done = cache.replace("k", "v0", "v1")
>> and based on this value, the application would branch in some way, and
>> so acquiring locks and waiting for each other is not enough, we can
>> only support this if write skew checks are enabled, and mandate the
>> full operation to rollback in the end. That might be one option, but I
>> really don't like to make it likely to rollback transactions, I'd
>> prefer to have an alternative like a new flag which enforces a "fresh
>> read" skipping the repeatable read guarantees. Of course this wouldn't
>> work if we're not actually sending the operations to the key owners,
>> so suspending the transaction is a much nicer approach from the user
>> perspective. Though I agree this behaviour should be selectable.
>>
>
> Ok, I'm slowly remembering your arguments... do you think the "fresh
> read" flag should be available for all operations, or does it make
> sense to make it an internal flag that only the atomic operations will
> use?
>
> To summarize, with this example:
> 1. Start tx
> 2. cache.get("k") -> "v0"
> 3. cache.replace("k", "v0", "v1")
> 4. gache.get("k") -> ??
> 5. Commit tx
>
> The options we could support are:
> a. Tx suspend: no retries, but also no rollback for replace() and 4)
> will not see the updated value

Right, this is what I trying to get to= earlier. If you went for the tx suspend option (particularly if this is done under the hood), this would certainly different behaviour to 4.x and looks quite dangerous TBH. However, this type of scenarios can be protected against from within the Infinispan.

> b. Optimistic locking + write skew check: if the key is modified by
> another tx between 2) and 5), the entire transaction has to be redone
> c. Optimistic locking + write skew check + fresh read: we only have to
> redo the tx if the key is modified by another tx between 3) and 5)

Infinispan can keep track of read keys and so it can use the fresh read option on an atomic operation if the key has been read previously in the transaction. So, these two options could be merged into 1 IMO.

> d. Pessimistic locking: if the key is modified between 2) and 5), the
> entire transaction has to be redone
> e. Pessimistic locking + fresh read: no redo, but decreased throughput
> because we hold the lock between 3) and 5)
>
> I guess there is no reason to support option d), as we're making an
> RPC to the owner in order to get the lock anyway.

Yeah, d) does not make sense cos if you go remote for a get("k"), you'd have to go remote for replace() too in order to be fully correct. A modification and L1 invalidation could have happened in an owner node for "k" in between 2) and 3)

> I think I'm leaning
> towards supporting only a) and e), but there might be cases where b)
> and c) would perform better.

Thanks for compiling these options.

So, my preference is for a), combined b+c) and e).

With http://community.jboss.org/docs/DOC-16973 in mind, it'd only be a) that would need adding. b+c) would be covered by the global OL/hybrid config and e) would be PL.

>
> Dan
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Sanne Grinovero-3
In reply to this post by Galder Zamarreno
2011/7/5 Galder Zamarreño <[hidden email]>:

>
> On Jul 5, 2011, at 11:46 AM, Sanne Grinovero wrote:
>
>> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>>
>>>
>>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>>
>>>> I agree they don't make sense, but only in the sense of exposed API
>>>> during a transaction: some time ago I admit I was expecting them to
>>>> just work: the API is there, nice public methods in the public
>>>> interface with javadocs explaining that that was exactly what I was
>>>> looking for, no warnings, no failures. Even worse, all works fine when
>>>> running a local test because how the locks currently work they are
>>>> acquired locally first, so unless you're running such a test in DIST
>>>> mode, and happen to be *not* the owner of the being tested key, people
>>>> won't even notice that this is not supported.
>>>>
>>>> Still being able to use them is very important, also in combination
>>>> with transactions: I might be running blocks of transactional code
>>>> (like a CRUD operation via OGM) and still require to advance a
>>>> sequence for primary key generation. This needs to be an atomic
>>>> operation, and I should really not forget to suspend the transaction.
>>>
>>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>>
>>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>>
>>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>>
>>> For example:
>>>
>>> Cache contains: k1=galder
>>>
>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>>> 3. Tx2 commits
>>> 4. Tx1 commits
>>> End result: k1=sanne
>>
>> Right.
>> To clarify, this is what would happen with the current implementation:
>>
>> 1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
>> returned "galder"
>> 2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
>> the scope of this transaction, but not seen by other tx
>> 3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
>> assigned, as because of repeatable read we're still seeing "galder"
>> 4. Tx2  & Tx1 commit
>>
>> ..and the end result depends on who commits first.
>
> The sequence of events above is what I suppose would happen with the suspended tx mode, not the current impl

thanks, I just felt the need to double check we where on the same page.

>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>>> 3. Tx2 rollback -> times out acquiring lock
>>> 4. Tx1 commits -> applies change
>>> End result: k1=sanne
>>
>> I'm not sure we're on the same line here. 1) should apply the
>> operation right away, so even if it might very briefly have to acquire
>> a lock on it, it's immediately released (not at the end of the
>> transaction), so why would TX2 have to wait for it to the point it
>> needs to rollback?
>
> This is what I was trying to picture as current implementation. It's true that it should apply the operation, but it also acquires the lock, at least in local mode and the locks are only release at prepare/commit time.
>
> Well, tx2 is trying to acquire a WL on a entry that's being modified by TX1. Here I'm assuming that Tx1 does 'something else' and so Tx2 times out waiting for the lock.
>
>>
>>
>>>
>>>>
>>>> Sanne
>>>>
>>>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>>>
>>>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>>>
>>>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>>>
>>>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>>>
>>>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>>>
>>>>>> Hello all,
>>>>>> some team members had a meeting yesterday, one of the discussed
>>>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>>>> Mircea just summarised it in the following proposal:
>>>>>>
>>>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>>>> well within the scope of optimistic transaction: this is because there
>>>>>> is a discrepancy between the value returned by the operation and the
>>>>>> value and the fact that the operation is applied or not:
>>>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>>>> the scope of the current transaction, but in fact there might be a
>>>>>> value committed by another transaction, hidden by the fact we're
>>>>>> running in repeatable read mode.
>>>>>> Later on, at prepare time when the same operation is applied on the
>>>>>> node that actually holds k, it might not succeed as another
>>>>>> transaction has updated k in between, but the return value of the
>>>>>> method was already evaluated long before this point.
>>>>>> In order to solve this problem, if an atomic operations happens within
>>>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>>>> remote node. This locks is held for the entire duration of the
>>>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>>>> keeping the lock remotely for potentially long time represents a
>>>>>> problem, the user can suspend the running transaction and run the
>>>>>> atomic operation out of transaction's scope, then resume the
>>>>>> transaction.
>>>>>>
>>>>>>
>>>>>> In addition to this, would would you think about adding a flag to
>>>>>> these methods which acts as suspending the transaction just before and
>>>>>> resuming it right after? I don't know what is the cost of suspending &
>>>>>> resuming a transaction, but such a flag could optionally be optimized
>>>>>> in future by just ignoring the current transaction instead of really
>>>>>> suspending it, or apply other clever tricks we might come across.
>>>>>>
>>>>>> I also think that we should discuss if such a behaviour should not be
>>>>>> the default - anybody using an atomic operation is going to make some
>>>>>> assumptions which are clearly incompatible with the transaction, so
>>>>>> I'm wondering what is the path here to "least surprise" for default
>>>>>> invocation.
>>>>>>
>>>>>> Regards,
>>>>>> Sanne
>>>>>> _______________________________________________
>>>>>> infinispan-dev mailing list
>>>>>> [hidden email]
>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>
>>>>> --
>>>>> Galder Zamarreño
>>>>> Sr. Software Engineer
>>>>> Infinispan, JBoss Cache
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> [hidden email]
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> --
>>> Galder Zamarreño
>>> Sr. Software Engineer
>>> Infinispan, JBoss Cache
>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> --
> Galder Zamarreño
> Sr. Software Engineer
> Infinispan, JBoss Cache
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Atomic operations and transactions

Emmanuel Bernard
Email summary:
Number of lines: 188
Number of useful lines (Strict): 1 (0,53%)
Number of useful line (contextual): 12 (6.3%)
Position of the useful line in the amount of data: 57 (had to scroll 30% of the data to find it and the addition 70% as I was not sure another one wasn't lost somewhere later)

I'm sure one can do better than that.

On 5 juil. 2011, at 15:15, Sanne Grinovero wrote:

> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>
>> On Jul 5, 2011, at 11:46 AM, Sanne Grinovero wrote:
>>
>>> 2011/7/5 Galder Zamarreño <[hidden email]>:
>>>>
>>>>
>>>> On Jul 4, 2011, at 11:25 AM, Sanne Grinovero wrote:
>>>>
>>>>> I agree they don't make sense, but only in the sense of exposed API
>>>>> during a transaction: some time ago I admit I was expecting them to
>>>>> just work: the API is there, nice public methods in the public
>>>>> interface with javadocs explaining that that was exactly what I was
>>>>> looking for, no warnings, no failures. Even worse, all works fine when
>>>>> running a local test because how the locks currently work they are
>>>>> acquired locally first, so unless you're running such a test in DIST
>>>>> mode, and happen to be *not* the owner of the being tested key, people
>>>>> won't even notice that this is not supported.
>>>>>
>>>>> Still being able to use them is very important, also in combination
>>>>> with transactions: I might be running blocks of transactional code
>>>>> (like a CRUD operation via OGM) and still require to advance a
>>>>> sequence for primary key generation. This needs to be an atomic
>>>>> operation, and I should really not forget to suspend the transaction.
>>>>
>>>> Fair point. At first glance, the best way to deal with this is suspending the tx cos that guarantees the API contract while not forcing locks to be acquired for too long.
>>>>
>>>> I'd advice though that whoever works on this though needs to go over existing use cases and see if the end result could differ somehow if this change gets applied. If any divergences are found and are to be expected, these need to be thoroughly documented.
>>>>
>>>> I've gone through some cases and end results would not differ at first glance if the atomic ops suspend the txs. The only thing that would change would be the expectations of lock acquisition timeouts by atomic ops within txs.
>>>>
>>>> For example:
>>>>
>>>> Cache contains: k1=galder
>>>>
>>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> suspends tx and applies change -> k1=sanne now
>>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> suspends tx and is not able to apply change
>>>> 3. Tx2 commits
>>>> 4. Tx1 commits
>>>> End result: k1=sanne
>>>
>>> Right.
>>> To clarify, this is what would happen with the current implementation:
>>>
>>> 1. Tx2 does a cache.get(k1) -> it reads the value of k1, and is
>>> returned "galder"
>>> 2. Tx1 does a cache.replace(k1, "galder", "sanne") -> k1="sanne" in
>>> the scope of this transaction, but not seen by other tx
>>> 3. Tx2 does a cache.replace(k1, "galder", "manik") -> k1="manik" is
>>> assigned, as because of repeatable read we're still seeing "galder"
>>> 4. Tx2  & Tx1 commit
>>>
>>> ..and the end result depends on who commits first.
>>
>> The sequence of events above is what I suppose would happen with the suspended tx mode, not the current impl
>
> thanks, I just felt the need to double check we where on the same page.
>
>>>> 1. Tx1 does a cache.replace(k1, "galder", "sanne") -> acquires lock
>>>> 2. Tx2 does a cache.replace(k1, "galder", "manik") -> waits for lock
>>>> 3. Tx2 rollback -> times out acquiring lock
>>>> 4. Tx1 commits -> applies change
>>>> End result: k1=sanne
>>>
>>> I'm not sure we're on the same line here. 1) should apply the
>>> operation right away, so even if it might very briefly have to acquire
>>> a lock on it, it's immediately released (not at the end of the
>>> transaction), so why would TX2 have to wait for it to the point it
>>> needs to rollback?
>>
>> This is what I was trying to picture as current implementation. It's true that it should apply the operation, but it also acquires the lock, at least in local mode and the locks are only release at prepare/commit time.
>>
>> Well, tx2 is trying to acquire a WL on a entry that's being modified by TX1. Here I'm assuming that Tx1 does 'something else' and so Tx2 times out waiting for the lock.
>>
>>>
>>>
>>>>
>>>>>
>>>>> Sanne
>>>>>
>>>>> 2011/7/4 Galder Zamarreño <[hidden email]>:
>>>>>> Do these atomic operations really make sense within an (optimitic) transaction?
>>>>>>
>>>>>> For example, putIfAbsent(): it stores a k,v pair if the key is not present. But the key about it's usability is that the return of putIfAbsent can tell you whether the put succeeded or not.
>>>>>>
>>>>>> Once you go into transactions, the result is only valid once the transaction has been prepared unless the pessimistic locking as per definition in http://community.jboss.org/docs/DOC-16973 is in use, and that's already pretty confusing IMO.
>>>>>>
>>>>>> I get the feeling that those atomic operations are particularly useful when transactions are not used cos they allow you to reduce to cache operations to one, hence avoiding the need to use a lock or synchronized block, or in our case, a transaction.
>>>>>>
>>>>>> On Jun 30, 2011, at 3:11 PM, Sanne Grinovero wrote:
>>>>>>
>>>>>>> Hello all,
>>>>>>> some team members had a meeting yesterday, one of the discussed
>>>>>>> subjects was about using atomic operations (putIfAbsent, etc..).
>>>>>>> Mircea just summarised it in the following proposal:
>>>>>>>
>>>>>>> The atomic operations, as defined by the ConcurrentHashMap, don't fit
>>>>>>> well within the scope of optimistic transaction: this is because there
>>>>>>> is a discrepancy between the value returned by the operation and the
>>>>>>> value and the fact that the operation is applied or not:
>>>>>>> E.g. putIfAbsent(k, v) might return true as there's no entry for k in
>>>>>>> the scope of the current transaction, but in fact there might be a
>>>>>>> value committed by another transaction, hidden by the fact we're
>>>>>>> running in repeatable read mode.
>>>>>>> Later on, at prepare time when the same operation is applied on the
>>>>>>> node that actually holds k, it might not succeed as another
>>>>>>> transaction has updated k in between, but the return value of the
>>>>>>> method was already evaluated long before this point.
>>>>>>> In order to solve this problem, if an atomic operations happens within
>>>>>>> the scope of a transaction, Infinispan eagerly acquires a lock on the
>>>>>>> remote node. This locks is held for the entire duration of the
>>>>>>> transaction, and is an expensive lock as it involves an RPC. If
>>>>>>> keeping the lock remotely for potentially long time represents a
>>>>>>> problem, the user can suspend the running transaction and run the
>>>>>>> atomic operation out of transaction's scope, then resume the
>>>>>>> transaction.
>>>>>>>
>>>>>>>
>>>>>>> In addition to this, would would you think about adding a flag to
>>>>>>> these methods which acts as suspending the transaction just before and
>>>>>>> resuming it right after? I don't know what is the cost of suspending &
>>>>>>> resuming a transaction, but such a flag could optionally be optimized
>>>>>>> in future by just ignoring the current transaction instead of really
>>>>>>> suspending it, or apply other clever tricks we might come across.
>>>>>>>
>>>>>>> I also think that we should discuss if such a behaviour should not be
>>>>>>> the default - anybody using an atomic operation is going to make some
>>>>>>> assumptions which are clearly incompatible with the transaction, so
>>>>>>> I'm wondering what is the path here to "least surprise" for default
>>>>>>> invocation.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Sanne
>>>>>>> _______________________________________________
>>>>>>> infinispan-dev mailing list
>>>>>>> [hidden email]
>>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>>
>>>>>> --
>>>>>> Galder Zamarreño
>>>>>> Sr. Software Engineer
>>>>>> Infinispan, JBoss Cache
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> infinispan-dev mailing list
>>>>>> [hidden email]
>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> [hidden email]
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>> --
>>>> Galder Zamarreño
>>>> Sr. Software Engineer
>>>> Infinispan, JBoss Cache
>>>>
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>> Galder Zamarreño
>> Sr. Software Engineer
>> Infinispan, JBoss Cache
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
12