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

On 4 May 2010, at 16:00, Galder Zamarreno wrote: > 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. Good point and thanks for the detailed analysis. Could you add this as a test? Further, does this happen when we use stores + repl/dist, or just stores + local mode?

Cheers Manik > > 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 > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev