10 a.m.
Digging further, I discovered that this issue does not affect replication. It only affects
cache stores.
This is because when replicating, TransactionXaAdapter keeps modifications as commands
executed, so the situation below, if applied to a replicated environment, would result on
the two modifications pointing to different atomic hash map instances. So, the effects of
the sending the first atomic hash map would have no effect on the second.
However, this is not the same with cache stores. Here, StoreModificationsBuilder comes
into play which transforms those commands into loaders.modifications.Modification built
with data inspected from the cache. Hence, those two commands end up with two
modifications that have retrieved the same atomic hash map from the cache. Hence, the
effects of replicating the first are seen by the second modification. This ultimately
leads to the issue below.
Regardless, I still think that there shouldn't be multiple modifications/commands for
the same thing here. It's wasteful for a start and has secondary effects as shown
below.
Cheers,
----- galder(a)redhat.com wrote:
Hi,
This is related to https://community.jboss.org/message/541023#541023
thread. Once https://jira.jboss.org/jira/browse/ISPN-418 fails,
another issue appears whereby a null delta is written to the cache
store. The reason for this is that the code in
testRestoreTransactionalAtomicMap actually generates two modifications
in the transaction.
The first one comes from the putIfAbsent below that can be found in
AtomicHashMap:
public static AtomicHashMap newInstance(Cache cache, Object
cacheKey) {
AtomicHashMap value = new AtomicHashMap();
Object oldValue = cache.putIfAbsent(cacheKey, value);
if (oldValue != null) value = (AtomicHashMap) oldValue;
return value;
}
And the second modification comes from
AtomicHashMapProxy.getDeltaMapForWrite call by the code below. This
comes from the map put call:
public V put(K key, V value) {
try {
startAtomic();
InvocationContext ctx = icc.createInvocationContext();
AtomicHashMap<K, V> deltaMapForWrite =
getDeltaMapForWrite(ctx);
return deltaMapForWrite.put(key, value);
}
finally {
endAtomic();
}
}
On one side, these two modifications do exactly the same, a put with
the atomic hash map, so it's not very efficient. One put would be
enough for the test to work. However, the reason the test fails is
after writing the first modification via:
public void writeObject(ObjectOutput output, Object subject)
throws IOException {
DeltaAware dw = (DeltaAware) subject;
output.writeObject(dw.delta());
}
dw.delta() call resets the delta and sets it to null. So, when the
second modification is written, delta is null and that's the final
value written. Obviously, when data is read, null delta is retrieved
and hence the test fails.
So IMO, one of the two modifications must go and my vote is for the
one in newInstance. I don't really understand why we should put when
creating a new instance of AtomicHashMap. It makes more sense to do
the put when you actually modify the map via a put call for example.
Manik, any idea why you added the putIfAbsent to newInstance?
As a side note, and this might be related, the javadoc of newInstance
needs updating since AtomicMapCache does not exist. It should probably
refer to AtomicMapLookup instead of AtomicMapCache.
/**
* Construction only allowed through this factory method. This
factory is intended for use internally by the
* CacheDelegate. User code should use {@link
org.infinispan.atomic.AtomicMapCache#getAtomicMap(Object)}.
*/
Cheers,
--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev