ISPN-7803 Functional commands-based AtomicMaps #5140

rvansa · 2017-05-18T13:02:28Z

https://issues.jboss.org/browse/ISPN-7803

When this gets in, I'll file PR to deprecate DeltaAware et all: https://github.com/rvansa/infinispan/tree/ISPN-7847

tristantarrant · 2017-05-19T07:33:15Z

documentation/src/main/asciidoc/upgrading/upgrading.asciidoc

+
+Infinispan now contains a new implementation of both `AtomicMap` and `FineGrainedAtomicMap`, but the semantics has been preserved. The new implementations don't use `DeltaAware` interface but link:../user_guide/user_guide.html#functional_map_api[Functional API] instead.
+
+There are no changes in needed for `AtomicMap`, but it now supports non-transactional use case as well.


remove 'in'

tristantarrant · 2017-05-19T07:33:58Z

documentation/src/main/asciidoc/upgrading/upgrading.asciidoc

+
+=== (FineGrained)AtomicMap reimplemented
+
+Infinispan now contains a new implementation of both `AtomicMap` and `FineGrainedAtomicMap`, but the semantics has been preserved. The new implementations don't use `DeltaAware` interface but link:../user_guide/user_guide.html#functional_map_api[Functional API] instead.


s/has/have/
s/don't/does not/
add a 'the' just before the functional api link

According to http://www.wordreference.com/definition/semantics 'semantics' is used with singular verb, despite it's plural noun.

tristantarrant · 2017-05-19T07:34:44Z

documentation/src/main/asciidoc/upgrading/upgrading.asciidoc

+
+There are no changes in needed for `AtomicMap`, but it now supports non-transactional use case as well.
+
+`FineGrainedAtomicMap` now uses link:../user_guide/user_guide.html#grouping[Grouping API] and therefore you need to link:../user_guide/user_guide.html#how_do_i_use_the_grouping_api[enable groups in configuration]. Also it holds entries as regular cache entries, plus one cache entry for cached key set (the map itself). Therefore the cache size or iteration/streaming results may differ. Note that fine grained atomic maps are still supported only on transactional caches.


add 'the' before the grouping api link
'still supported on transactional caches only'

galderz · 2017-05-24T11:00:21Z

core/src/main/java/org/infinispan/atomic/MergeOnStore.java

+    * @param other Actual value stored in the cache, or <code>null</code> if the entry does not exist.
+    * @return Value that will be stored in the cache, or null if it should be removed.
+    */
+   Object merge(Object other);


No generics?

No. ReadCommittedEntry won't use any generics anyway. The implementation should expect any type of object stored in the cache. I don't see much value in that.

galderz · 2017-05-24T11:05:08Z

core/src/main/java/org/infinispan/atomic/impl/AtomicKeySetImpl.java

+
+      @Override
+      public Void apply(EntryView.ReadWriteEntryView<Object, Object> view) {
+         if (view.find().isPresent()) {


Optional.map(f).orElseThrow() ? :)

Since you call view.set(), that would create another object capturing the view.

galderz · 2017-05-24T11:14:44Z

core/src/main/java/org/infinispan/atomic/impl/TransactionHelper.java

+      }
+   }
+
+   <T> T run(Supplier<T> op) {


This is not implementing the correct pattern of dealing with transaction commit/rollback. This code shows the correct pattern.

Then we should fix CacheImpl.executeCommandAndCommitIfNeeded and some code in functional maps as well... this is basically inlined ^.

Fixed TransactionHelper

galderz · 2017-05-24T11:22:44Z

Have you benchmarked this and compared with old method?

rvansa · 2017-05-26T08:57:01Z

No, regreattably I haven't had a chance to write a benchmark. It will mostly depend on performance of functional commands in general. However I can't argue that it will change dramatically after ISPN-7590 since I don't use ReadWriteKeyValueCommand which propagates the previous value to backup.

rvansa · 2017-05-26T12:03:52Z

Rebased.

galderz · 2017-05-29T10:48:04Z

Thx for replies, all looks good. Anyone else wants to look at this?

rvansa · 2017-06-01T13:30:47Z

@galderz Seems there are no further objections.

wburns

Sorry just had a couple minor things. Looks good to me though @rvansa Especially with the interceptor fixes. I assume your other PR hasn't found issues with them? :)

wburns · 2017-05-25T14:47:45Z

core/src/main/java/org/infinispan/atomic/AtomicMap.java

 import org.infinispan.util.concurrent.IsolationLevel;

 /**
 * This is a special type of Map geared for use in Infinispan.  AtomicMaps have two major characteristics:
 *
 * <ol>
 *    <li>Atomic locking and isolation over the entire collection</li>
- *    <li>Fine-grained serialization of deltas</li>
+ *    <li>Replication of updates through deltas</li>


Do we still want to use the term delta?

I think that 'delta' is a general term, synonym for a 'change'.

wburns · 2017-05-25T14:48:55Z

core/src/main/java/org/infinispan/atomic/AtomicMapLookup.java

@@ -142,6 +129,7 @@
    * @param <MK>  key param of the cache
    */
   public static <MK> void removeAtomicMap(Cache<MK, ?> cache, MK key) {
-      cache.getAdvancedCache().withFlags(Flag.IGNORE_RETURN_VALUES).remove(key);
+      // This removes any entry from the cache but includes special handling for fine-grained maps
+      FineGrainedAtomicMapProxyImpl.removeMap((Cache<Object, Object>) cache, key);


I assume this is fine even if it is not fine grained? I just ask because the create specifically had 2 branches.

Yes, for anything but fine grained it works just like a regular cache.remove(). Non-fine grained maps are just single entries, so no problem with that.

* LockingMode.SKIP is equivalent to Flag.SKIP_LOCKING * ExecutionMode.LOCAL is equivalent to Flag.CACHE_MODE_LOCAL * encoding of Params is still just single byte * Params are internally converted to flags bit set to simplify checks * FunctionalMapImpl attempts to retrieve flags from the provided Cache * ReadOnly*Commands should honour Params

…lying implementation * no fine-grained maps so far

…internals * Not changing AdvancedCache.getGroup as this is API & SPI * Changed Grouper SPI but since existing implementations are expected to be used only with string-based keys the bridge implementation should work.

* require transactional cache with grouping enabled * store map entries as cache-entries => expected number of entries changed in some tests * introduce MergeOnStore and minor changes in CommitManager

rvansa · 2017-06-01T15:20:46Z

Rebased (this had conflicts with removal of Metadata parameter from CommitManager).

rvansa · 2017-06-06T13:07:34Z

@galderz @wburns anything left?

galderz · 2017-06-06T15:49:22Z

CI running... let's wait for it

rvansa · 2017-06-08T06:54:15Z

@galderz CI finished...

wburns · 2017-06-08T12:33:18Z

I see there are a bunch of failures with DirectoryProviderModuleMemberRegistrationIT. Is this just a random failure that occurs sometimes? Unfortunately I can't tell as easily with Jenkins. I wish I could see the history of a given test.

http://ci.infinispan.org/job/Infinispan/job/PR-5140/33/display/redirect

I will poke around, maybe those are using atomic map? I wasn't aware of indexing doing such a thing though.

wburns · 2017-06-08T13:13:35Z

It seems they failed here as well http://ci.infinispan.org/job/Infinispan/job/PR-5183/4/#showFailuresLink So it does seem unrelated, guess we have more failures in the test suite now 👎

wburns · 2017-06-08T13:16:30Z

@rvansa is there anything you wanted to squash/rebase before I merged this, there are quite a few commits :)

rvansa · 2017-06-08T13:20:59Z

@wburns No, please keep the commits as-is. Those are logically structured changes.

wburns · 2017-06-08T13:29:17Z

Integrated into master, thanks @rvansa !

danberindei · 2017-06-12T12:10:50Z

core/src/test/java/org/infinispan/api/mvcc/repeatable_read/WriteSkewTest.java

@@ -57,6 +57,7 @@ public void setUp() {
      // The default cache is NOT write skew enabled.
      cacheManager = TestCacheManagerFactory.createCacheManager(configurationBuilder);
      configurationBuilder.locking().isolationLevel(IsolationLevel.REPEATABLE_READ);
+      configurationBuilder.clustering().hash().groups().enabled();


@rvansa This fails with trace logging enabled, because it forces the component registry to create a DistributionManagerImpl, and its start() methods throws a NPE if the cache manager is not clustered. Do you really need grouping in this class?

testDontFailOnImmediateRemovalOfAtomicMaps uses fine grained maps, so grouping is necessary. Please use localAddress in the trace log instead, and drop the getAddress() method.

danberindei

Some late comments

danberindei · 2017-06-14T09:26:31Z

commons/src/main/java/org/infinispan/commons/api/functional/Param.java

+    * its correctness. If the application is sure that no concurrent operation occurs,
+    * it is possible to increase performance by setting this param to {@link #SKIP}.
+    * The result of any operation without locking is undefined under presence of concurrent
+    * writes.


Missing @since tag.

danberindei · 2017-06-14T09:26:55Z

commons/src/main/java/org/infinispan/commons/api/functional/Param.java

+   }
+
+   /**
+    * Defines where is the command executed.


Missing @since tag.

danberindei · 2017-06-14T09:36:23Z

core/src/main/java/org/infinispan/atomic/MergeOnStore.java

+ * only with {@link org.infinispan.context.Flag#SKIP_CACHE_LOAD} and
+ * {@link org.infinispan.context.Flag#SKIP_CACHE_STORE} or implementing a custom externalizer that will deal with this.
+ */
+public interface MergeOnStore {


This looks a lot like Delta to me... do we really want to expose another public API that does the same thing, instead of using a lambda?

I'm not sure what you mean about the atomicity of load and store into persistence layer, don't we allow using atomic maps with persistence?

Also, missing @since tag.

Delta was supposed to work in general, this contract is more narrow - speaking only about atomic application into DC. It is a way to allow the FGAM use case - modifying an entry without having it locked.

FGAMs with persistence would get broken in the old implementation.

rvansa added the Ready for Review label May 18, 2017

rvansa requested review from galderz and danberindei May 18, 2017 13:02

tristantarrant reviewed May 19, 2017

View reviewed changes

rvansa force-pushed the ISPN-7803 branch 3 times, most recently from 18fee71 to d1a8b73 Compare May 19, 2017 08:44

ryanemerson added Needs Rebase and removed Ready for Review labels May 23, 2017

galderz reviewed May 24, 2017

View reviewed changes

rvansa force-pushed the ISPN-7803 branch from d1a8b73 to e8de944 Compare May 26, 2017 12:02

rvansa added Ready for Review and removed Needs Rebase labels May 26, 2017

wburns reviewed Jun 1, 2017

View reviewed changes

rvansa added 7 commits June 1, 2017 17:15

ISPN-7803 Fix bugs in functional commands behaviour in transaction

b1ad479

ISPN-7803 Fix OptimisticTxAtomicLocking test waiting until timeout

5773d1d

ISPN-7803 AtomicMap implementation using functional API for the under…

2247145

…lying implementation * no fine-grained maps so far

ISPN-7803 Fine-grained atomic maps

d2a20d2

* require transactional cache with grouping enabled * store map entries as cache-entries => expected number of entries changed in some tests * introduce MergeOnStore and minor changes in CommitManager

ISPN-7803 Remove old AtomicHashMap implementation

43d71b6

ISPN-7803 Document changes in Upgrading guide

4477812

rvansa force-pushed the ISPN-7803 branch from e8de944 to 4477812 Compare June 1, 2017 15:19

tristantarrant modified the milestones: 9.1.0.Final, 9.1.0.Beta1 Jun 8, 2017

wburns merged commit 738cd9e into infinispan:master Jun 8, 2017

danberindei reviewed Jun 12, 2017

View reviewed changes

danberindei reviewed Jun 14, 2017

View reviewed changes


		Infinispan now contains a new implementation of both `AtomicMap` and `FineGrainedAtomicMap`, but the semantics has been preserved. The new implementations don't use `DeltaAware` interface but link:../user_guide/user_guide.html#functional_map_api[Functional API] instead.

		There are no changes in needed for `AtomicMap`, but it now supports non-transactional use case as well.


		=== (FineGrained)AtomicMap reimplemented

		Infinispan now contains a new implementation of both `AtomicMap` and `FineGrainedAtomicMap`, but the semantics has been preserved. The new implementations don't use `DeltaAware` interface but link:../user_guide/user_guide.html#functional_map_api[Functional API] instead.


		There are no changes in needed for `AtomicMap`, but it now supports non-transactional use case as well.

		`FineGrainedAtomicMap` now uses link:../user_guide/user_guide.html#grouping[Grouping API] and therefore you need to link:../user_guide/user_guide.html#how_do_i_use_the_grouping_api[enable groups in configuration]. Also it holds entries as regular cache entries, plus one cache entry for cached key set (the map itself). Therefore the cache size or iteration/streaming results may differ. Note that fine grained atomic maps are still supported only on transactional caches.

ISPN-7803 Functional commands-based AtomicMaps #5140

ISPN-7803 Functional commands-based AtomicMaps #5140

Conversation

rvansa commented May 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galderz commented May 24, 2017

rvansa commented May 26, 2017

rvansa commented May 26, 2017

galderz commented May 29, 2017

rvansa commented Jun 1, 2017

wburns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvansa commented Jun 1, 2017

rvansa commented Jun 6, 2017

galderz commented Jun 6, 2017

rvansa commented Jun 8, 2017

wburns commented Jun 8, 2017 • edited

wburns commented Jun 8, 2017

wburns commented Jun 8, 2017

rvansa commented Jun 8, 2017

wburns commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danberindei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvansa commented May 18, 2017 •

edited

wburns commented Jun 8, 2017 •

edited