ISPN-8670 Standalone Server Cache Storage #5666

tristantarrant · 2018-01-10T11:56:04Z

https://issues.jboss.org/browse/ISPN-8670
https://issues.jboss.org/browse/ISPN-8685

galderz

Just a minor comment.

galderz · 2018-01-12T07:36:01Z

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

@@ -1027,6 +1028,14 @@ else if (ch == 'f')
      return out.toString();
   }

+   public static <T> Supplier<T> getInstanceSupplier(Class<T> klass) {


Why not Function instead of Supplier? By using a Supplier, you're capturing a variable outside the lambda (klass). Could you have a Function that takes Class<T> and returns T? Then the function would be pure.

Because in the context of the server LocalConfigurationStorage, I need to be able to set a bunch of context.

galderz · 2018-01-12T07:36:32Z

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

+      return () -> getInstance(klass);
+   }
+
+   public static <T> Supplier<T> getInstanceSupplier(String className, ClassLoader classLoader) {


Same thing but with a BiFunction.

galderz · 2018-01-15T09:31:08Z

Needs rebase

tristantarrant · 2018-01-16T05:46:39Z

Failures are unrelated

- add an enum-based ConfigurationStorage type for cleaner API/XML configuration - rename the EmbeddedLocalConfigurationStorage to OverlayLocalConfigurationStorage (as it can be used for servers too) - add a VolatileLocalConfigurationStorage

ryanemerson

Overall LGTM, just a few minor remarks.

ryanemerson · 2018-01-16T09:56:56Z

.../org/jboss/as/clustering/infinispan/subsystem/StandaloneServerLocalConfigurationStorage.java

+public class StandaloneServerLocalConfigurationStorage implements ServerLocalConfigurationStorage {
+   private static final String[] CACHE_MODES = {"local-cache", "invalidation-cache", "replicated-cache", "distributed-cache", "scattered-cache"};
+   private ModelControllerClient modelControllerClient;
+   PathAddress rootPath;


Can be private

ryanemerson · 2018-01-16T09:57:10Z

.../org/jboss/as/clustering/infinispan/subsystem/StandaloneServerLocalConfigurationStorage.java

+   private static final String[] CACHE_MODES = {"local-cache", "invalidation-cache", "replicated-cache", "distributed-cache", "scattered-cache"};
+   private ModelControllerClient modelControllerClient;
+   PathAddress rootPath;
+   private EmbeddedCacheManager embeddedCacheManager;


ryanemerson · 2018-01-16T10:05:51Z

.../org/jboss/as/clustering/infinispan/subsystem/StandaloneServerLocalConfigurationStorage.java

+      }
+      // Nothing found
+      return null;
+   }


Do we prevent caches/templates of different types having the same name? If not then this method will always retrieve the first template encountered based upon the CACHE_MODES order.

We should prevent it (that's the behaviour in embedded anyway), but this is not the PR to do it

ryanemerson · 2018-01-16T10:08:28Z

.../org/jboss/as/clustering/infinispan/subsystem/StandaloneServerLocalConfigurationStorage.java

+      String cacheMode = findCacheMode(name, false);
+
+      if (cacheMode == null) {
+         throw new CacheConfigurationException("No such cache");


"No Such ..." doesn't sound right to me (although I think we have similar messages elsewhere), "Cache '%s' does not exist", name is more informative and clear IMO. The same also applies in createCache(..).

ryanemerson · 2018-01-16T10:18:38Z

core/src/main/java/org/infinispan/globalstate/impl/OverlayLocalConfigurationStorage.java

-      this.globalConfiguration = cacheManager.getCacheManagerConfiguration();
-      this.cacheManager = cacheManager;
-      this.parserRegistry = new ParserRegistry();
+   public OverlayLocalConfigurationStorage() {


Redundant constructor

ryanemerson · 2018-01-16T10:21:06Z

core/src/main/java/org/infinispan/globalstate/impl/VolatileLocalConfigurationStorage.java

+   protected GlobalConfiguration globalConfiguration;
+
+   public VolatileLocalConfigurationStorage() {
+   }


Redundant constructor

ryanemerson · 2018-01-16T10:29:20Z

core/src/main/java/org/infinispan/globalstate/impl/OverlayLocalConfigurationStorage.java

+   private ConcurrentHashSet<String> persistentCaches = new ConcurrentHashSet<>();
+
+   public OverlayLocalConfigurationStorage() {
+   }


Redundant constructor

ryanemerson · 2018-01-16T10:35:27Z

...pan/src/main/java/org/jboss/as/clustering/infinispan/subsystem/CacheContainerAddHandler.java

+                        globalStateBuilder.setConfigurationStorageClass(StandaloneServerLocalConfigurationStorage.class.getName());
+                        break;
+                    case DOMAIN_SERVER:
+                        // TODO: warn that we won't be persisting the changes to the server model


I think this is sufficient

tristantarrant · 2018-01-16T11:11:18Z

@ryanemerson I have addressed all issues. I have also converted the "throw new" to use i18n messages.

…rvers

…persistent location

- also adds an integration test to exercise securely invoking admin ops

tristantarrant · 2018-01-16T18:40:34Z

Failures unrelated

ryanemerson · 2018-01-17T09:31:20Z

@tristantarrant Merged, thanks!

tristantarrant added this to the 9.2.0.CR1 milestone Jan 10, 2018

galderz approved these changes Jan 12, 2018

View reviewed changes

galderz added the Needs Rebase label Jan 15, 2018

tristantarrant force-pushed the ISPN-8670/standalone_cache_admin branch from 06dfcdd to 3589970 Compare January 15, 2018 12:10

tristantarrant removed the Needs Rebase label Jan 15, 2018

tristantarrant force-pushed the ISPN-8670/standalone_cache_admin branch 2 times, most recently from 12d0dfd to 7dca1d1 Compare January 15, 2018 20:36

tristantarrant force-pushed the ISPN-8670/standalone_cache_admin branch from 7dca1d1 to 8fb59d8 Compare January 16, 2018 09:39

ryanemerson reviewed Jan 16, 2018

View reviewed changes

tristantarrant force-pushed the ISPN-8670/standalone_cache_admin branch from 8fb59d8 to b7c8aeb Compare January 16, 2018 11:09

tristantarrant added 3 commits January 16, 2018 15:05

ISPN-8670 Implement a managed configuration storage for standalone se…

e431f23

…rvers

ISPN-8670 Lock the persistent global state location and add a shared …

f41f389

…persistent location

ISPN-8685 Ensure admin tasks apply security

92a8223

- also adds an integration test to exercise securely invoking admin ops

tristantarrant force-pushed the ISPN-8670/standalone_cache_admin branch from b7c8aeb to 92a8223 Compare January 16, 2018 14:05

ryanemerson merged commit 59cd718 into infinispan:master Jan 17, 2018

tristantarrant deleted the ISPN-8670/standalone_cache_admin branch January 21, 2020 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-8670 Standalone Server Cache Storage #5666

ISPN-8670 Standalone Server Cache Storage #5666

tristantarrant commented Jan 10, 2018 •

edited

galderz left a comment

galderz Jan 12, 2018

tristantarrant Jan 12, 2018

galderz Jan 12, 2018

galderz commented Jan 15, 2018

tristantarrant commented Jan 16, 2018

ryanemerson left a comment

ryanemerson Jan 16, 2018

ryanemerson Jan 16, 2018

ryanemerson Jan 16, 2018

tristantarrant Jan 16, 2018

ryanemerson Jan 16, 2018

ryanemerson Jan 16, 2018

ryanemerson Jan 16, 2018

ryanemerson Jan 16, 2018

ryanemerson Jan 16, 2018

tristantarrant commented Jan 16, 2018

tristantarrant commented Jan 16, 2018

ryanemerson commented Jan 17, 2018

ISPN-8670 Standalone Server Cache Storage #5666

ISPN-8670 Standalone Server Cache Storage #5666

Conversation

tristantarrant commented Jan 10, 2018 • edited

galderz 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

galderz commented Jan 15, 2018

tristantarrant commented Jan 16, 2018

ryanemerson 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

tristantarrant commented Jan 16, 2018

tristantarrant commented Jan 16, 2018

ryanemerson commented Jan 17, 2018

tristantarrant commented Jan 10, 2018 •

edited