ISPN-3048 Eviction needs to be transactional #2252
Conversation
| return controlledPassivationManager; | ||
| } | ||
|
|
||
| protected final Future<Object> putWithFuture(final Object key, final Object value) { |
|
@pruivo Not sure if you noticed, but I previously created |
|
@galderz thanks for the heads up. I take a look to |
|
Sure, you can remove it. |
|
updated! |
|
pulling.. |
|
EvictionWithPassivationAndConcurrentOperationsTest>EvictionWithConcurrentOperationsTest.testScenario6:352->assertInMemory:44 Wrong value for key key1 in data container expected: but was: |
|
@mmarkus do you have any logs about it? I cannot reproduce it :( |
| cdl.commitEntry(e, null, cmd, ctx); | ||
| removeFromStoreIfNeeded(key); | ||
| try { | ||
| cdl.lock(key); |
There was a problem hiding this comment.
Good catch if 2 threads come in and 1 activates before the other can check the DC.
|
close for now. I need to change something... |
|
some changes:
|
| // ensure keys are properly locked for evict commands | ||
| command.setFlags(Flag.ZERO_LOCK_ACQUISITION_TIMEOUT, Flag.CACHE_MODE_LOCAL); | ||
| try { | ||
| lockKey(ctx, command.getKey(), 0, command.hasFlag(Flag.SKIP_LOCKING)); |
There was a problem hiding this comment.
Have you verified it is safe to no longer lock in a tx for an evict command? I am not aware of why we would require locking other than in passivation, do you know?
There was a problem hiding this comment.
I see the non tx interceptor did something similar as well.
There was a problem hiding this comment.
that code was outdated. Previously, all the nodes acquire the locks for all the operations (non-tx caches) and transactions (tx caches). But then, we introduce the primary owner concept (to reduce the deadlocks) and only one node started to lock.
In that code, it makes no sense to acquire the lock in the backup/non owner because the lock is only acquired in the primary owner.
The synchronization between EvictCommand and operations/transactions is done via CDL.lock()
There was a problem hiding this comment.
I agree, just wanted to make sure what was going on. Thanks.
|
@wburns updated :) |
| lock(key, false); | ||
| break; | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
I am thinking if we want this to not support interruption, then just set a boolean variable to symbolize it was interrupted and after this finally acquires the lock, interrupt the thread before returning from this method. Then we don't have to throw a RuntimeException either, which seems suspicious.
* Fixed issues with manual eviction to make it more reliable * Added tests for manual and size based eviction which happens concurrently with other operations
|
Pulling. |
|
Integrated, thanks @pruivo ! |
concurrently with other operations
Notes:
https://issues.jboss.org/browse/ISPN-3048