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