ISPN-8078 Support functional commands in scattered mode #5316

rvansa · 2017-07-25T10:07:44Z

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

Depends on #5312

karesti

It's super hard for me to understand all the code here, I added some minor comments.

karesti · 2017-07-26T07:12:53Z

core/src/test/java/org/infinispan/scattered/ScatteredSyncFuncTest.java

+      assertEquals(null, cache(1, cacheName).merge(key, "x", (o, n) -> "abc".equals(o) ? null : "unexpected"));
+      assertLocalValue(0, key, null);
+      assertLocalValue(1, key, null);
+      assertLocalValue(2, key, "ab");


why is the value not modified into "unexpected" here ?

ok, this is because is the local value and the cal was done from node 1, is that ok ?

yes, the merge should be run only on originator (1) and owner (0).

karesti · 2017-07-26T07:30:22Z

core/src/test/java/org/infinispan/functional/AbstractFunctionalTest.java

@@ -43,8 +48,15 @@ protected void createCacheManagers() throws Throwable {
      replBuilder.clustering().cacheMode(isSync ? CacheMode.REPL_SYNC : CacheMode.REPL_ASYNC);
      configureCache(replBuilder);
      cacheManagers.stream().forEach(cm -> cm.defineConfiguration(REPL, replBuilder.build()));
+      // Create scattered caches
+      if (!Boolean.TRUE.equals(transactional)) {


Personally I would init to Boolean.FALSE the MultipleCacheManagersTest and just test !transactional. Or in this case do Boolean.FALSE.equals(transactional)

I can init the transactional to false in the constructor... I think there was some reason to have this as a tri-state, maybe related to @InTransactionalMode.

karesti · 2017-07-26T07:34:53Z

core/src/test/java/org/infinispan/functional/AbstractFunctionalTest.java

@@ -94,6 +106,8 @@ private void initMaps() {
      fmapD2 = FunctionalMapImpl.create(cacheManagers.get(1).<Object, String>getCache(DIST).getAdvancedCache());
      fmapR1 = FunctionalMapImpl.create(cacheManagers.get(0).<Object, String>getCache(REPL).getAdvancedCache());
      fmapR2 = FunctionalMapImpl.create(cacheManagers.get(1).<Object, String>getCache(REPL).getAdvancedCache());
+      fmapS1 = FunctionalMapImpl.create(cacheManagers.get(0).<Object, String>getCache(SCATTERED).getAdvancedCache());


not related, but I noticed when I've been working on encoding+tx in functional maps.
Did you notice that fmapL1 and fmapL2 work on the same cache ? Is this normal ? I don't understand why it's like that. When I did a quick test to use 0 and 1, I got errors, but well, I don't know if this is normal or not ... I didn't dig more yet.

It seems to me that the whole class hierarchy for functional tests would use some refactoring, having only fmap1 and fmap2 and not all those by-cache-mode fields. I guess that all local mode ops should use only fmapL1, because in non-local caches the commands are not sent to any other node.

karesti · 2017-07-26T07:39:52Z

core/src/main/java/org/infinispan/interceptors/distribution/BaseDistributionInterceptor.java

-         BiConsumer<ArrayIterator, Object> consumer = ArrayIterator::add;
-         BiConsumer<ArrayIterator, ArrayIterator> combiner = ArrayIterator::combine;
-         ((Stream) rv).collect(supplier, consumer, combiner);
+         ((Stream) rv).collect(new ArrayCollector(allFuture.results));


rv is Always a Stream ? Impossible to get a ClassCastException here ?

Yes, ReadOnlyManyCommand.perform() returns a Stream. If we get something else, there's not much else to do about it than throw an exception.

karesti · 2017-07-26T07:42:34Z

commons/src/main/java/org/infinispan/commons/util/ArrayCollector.java

+import java.util.function.Function;
+import java.util.function.Supplier;
+
+public class ArrayCollector implements java.util.stream.Collector<Object, ArrayCollector, ArrayCollector>, Supplier<ArrayCollector> {


As this is an utility class now and not a private class, could be nice to test it unitary and document it uses ?

karesti · 2017-07-26T07:44:11Z

commons/src/main/java/org/infinispan/commons/util/ArrayCollector.java

+   }
+
+   public void add(Object item) {
+      array[pos] = item;


calling this method too much could lead into ArrayIndexOutOfBounds.

Not much else I could do if the array is not sized properly.

karesti · 2017-07-26T07:56:45Z

core/src/main/java/org/infinispan/commands/functional/WriteOnlyManyEntriesCommand.java

@@ -93,7 +96,8 @@ public Object perform(InvocationContext ctx) throws Throwable {

   @Override
   public boolean isReturnValueExpected() {
-      return false;
+      // Scattered cache always needs some response.


but for this case, the return is null ... isn't it ?

In scattered cache it will return an array of InternalCacheValue (after modification), because originator cannot apply the function locally (it does not have the previous version).

karesti · 2017-07-26T11:53:52Z

...src/main/java/org/infinispan/interceptors/distribution/ScatteredDistributionInterceptor.java

         if (command.hasAnyFlag(FlagBitSets.PUT_FOR_STATE_TRANSFER)) {
-            contextEntry.setMetadata(command.getMetadata());
+            // we don't increment versions with state transfer


why check to do nothing ?

I could use if (info.isPrimary() && !command.hasAnyFlag(FlagBitSets.PUT_FOR_STATE_TRANSFER))...

tristantarrant · 2017-08-02T13:57:09Z

Temporarily closing because of CI overload

* Also move BaseDistributionInterceptor$ArrayIterator -> commons/ArrayCollector

* Scattered cache now uses RepeatableReadEntries to support concurrent changes in multi-key operations

rvansa · 2017-09-25T12:42:17Z

@karesti Rebased, could you re-check?

galderz · 2017-09-27T14:27:10Z

CI looks good, anything else to add here?

rvansa · 2017-09-27T14:59:12Z

@galderz Not from me...

karesti

LGTM :)

tristantarrant · 2017-09-28T18:46:02Z

Merged !

rvansa requested a review from karesti July 25, 2017 10:07

rvansa added the Ready for Review label Jul 25, 2017

karesti requested changes Jul 26, 2017

View reviewed changes

karesti approved these changes Jul 27, 2017

View reviewed changes

tristantarrant closed this Aug 2, 2017

rvansa reopened this Sep 25, 2017

rvansa added 2 commits September 25, 2017 11:58

ISPN-8078 Refactor helpers out of NonTxDistributionInterceptor

4f5c52e

* Also move BaseDistributionInterceptor$ArrayIterator -> commons/ArrayCollector

ISPN-8078 Support functional commands in scattered mode

8fc071a

* Scattered cache now uses RepeatableReadEntries to support concurrent changes in multi-key operations

rvansa force-pushed the ISPN-8078 branch from a5eeab0 to 8fc071a Compare September 25, 2017 12:41

karesti approved these changes Sep 27, 2017

View reviewed changes

tristantarrant merged commit b3fc0fc into infinispan:master Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-8078 Support functional commands in scattered mode #5316

ISPN-8078 Support functional commands in scattered mode #5316

rvansa commented Jul 25, 2017 •

edited

karesti left a comment

karesti Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017

karesti Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017

karesti Jul 26, 2017

rvansa Jul 26, 2017 •

edited

tristantarrant commented Aug 2, 2017

rvansa commented Sep 25, 2017

galderz commented Sep 27, 2017

rvansa commented Sep 27, 2017

karesti left a comment

tristantarrant commented Sep 28, 2017

ISPN-8078 Support functional commands in scattered mode #5316

ISPN-8078 Support functional commands in scattered mode #5316

Conversation

rvansa commented Jul 25, 2017 • edited

karesti 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

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

rvansa Jul 26, 2017 • edited

Choose a reason for hiding this comment

tristantarrant commented Aug 2, 2017

rvansa commented Sep 25, 2017

galderz commented Sep 27, 2017

rvansa commented Sep 27, 2017

karesti left a comment

Choose a reason for hiding this comment

tristantarrant commented Sep 28, 2017

rvansa commented Jul 25, 2017 •

edited

rvansa Jul 26, 2017 •

edited