geode-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (GEODE-3007) Simplify support for custom GFSH commands
Date Mon, 06 Nov 2017 16:34:01 GMT

    [ https://issues.apache.org/jira/browse/GEODE-3007?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16240527#comment-16240527 ] 

ASF GitHub Bot commented on GEODE-3007:
---------------------------------------

jdeppe-pivotal closed pull request #1011: GEODE-3007: Simplify support for custom GFSH commands
URL: https://github.com/apache/geode/pull/1011
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/geode-assembly/build.gradle b/geode-assembly/build.gradle
index 1f139a5ac9..146df75382 100755
--- a/geode-assembly/build.gradle
+++ b/geode-assembly/build.gradle
@@ -160,6 +160,7 @@ def cp = {
         // depedencies from geode-core
         it.contains('antlr') ||
         it.contains('commons-io') ||
+        it.contains('commons-beanutils') ||
         it.contains('commons-collections') ||
         it.contains('commons-lang') ||
         it.contains('commons-logging') ||
@@ -168,6 +169,7 @@ def cp = {
         it.contains('commons-codec') ||
         it.contains('fast-classpath-scanner') ||
         it.contains('fastutil') ||
+        it.contains('guava') ||
         it.contains('jackson-annotations') ||
         it.contains('jackson-core') ||
         it.contains('jackson-databind') ||
@@ -193,19 +195,20 @@ def cp = {
         it.contains('log4j-slf4j-impl') ||
         it.contains('shiro') ||
         it.contains('slf4j-api') ||
+        it.contains('spring-beans') ||
+        it.contains('spring-context') ||
+        it.contains('spring-context-support') ||
         it.contains('spring-core') ||
         it.contains('spring-shell') ||
         it.contains('snappy') ||
         it.contains('jgroups') ||
         it.contains('netty') ||
-
         // dependencies from geode-lucene
         it.contains('lucene-analyzers-common') ||
         it.contains('lucene-core') ||
         it.contains('lucene-queries') ||
         it.contains('lucene-queryparser') ||
         it.contains('lucene-analyzers-phonetic') ||
-
         // dependencies from geode-protobuf
         it.contains('protobuf-java')
       }
diff --git a/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java b/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java
index a67dddcdd7..94ddc8f322 100644
--- a/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java
+++ b/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java
@@ -18,9 +18,9 @@
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
+import org.apache.geode.test.junit.categories.AcceptanceTest;
 import org.apache.geode.test.junit.rules.gfsh.GfshRule;
 import org.apache.geode.test.junit.rules.gfsh.GfshScript;
-import org.apache.geode.test.junit.categories.AcceptanceTest;
 
 @Category(AcceptanceTest.class)
 public class StatusLocatorRealGfshTest {
@@ -30,7 +30,6 @@
   @Test
   public void statusLocatorSucceedsWhenConnected() throws Exception {
     GfshScript.of("start locator --name=locator1").execute(gfshRule);
-
     GfshScript.of("connect", "status locator --name=locator1").execute(gfshRule);
   }
 
diff --git a/geode-assembly/src/test/resources/expected_jars.txt b/geode-assembly/src/test/resources/expected_jars.txt
index bc49bcef62..7e3d438720 100644
--- a/geode-assembly/src/test/resources/expected_jars.txt
+++ b/geode-assembly/src/test/resources/expected_jars.txt
@@ -64,6 +64,7 @@ spring-aop
 spring-aspects
 spring-beans
 spring-context
+spring-context-support
 spring-core
 spring-expression
 spring-hateoas
diff --git a/geode-core/build.gradle b/geode-core/build.gradle
index 3f47b0784e..263cc33138 100755
--- a/geode-core/build.gradle
+++ b/geode-core/build.gradle
@@ -52,6 +52,7 @@ dependencies {
   compile 'commons-io:commons-io:' + project.'commons-io.version'
   compile 'commons-validator:commons-validator:' + project.'commons-validator.version'
   compile 'commons-digester:commons-digester:' + project.'commons-digester.version'
+  compile 'com.google.guava:guava:' + project.'guava.version'
 
   compile 'commons-lang:commons-lang:' + project.'commons-lang.version'
   compile ('commons-modeler:commons-modeler:' + project.'commons-modeler.version') {
@@ -111,9 +112,7 @@ dependencies {
     exclude module: 'aopalliance'
     exclude module: 'asm'
     exclude module: 'cglib'
-    exclude module: 'guava'
     exclude module: 'spring-aop'
-    exclude module: 'spring-context-support'
     ext.optional = true
   }
   compile ('org.iq80.snappy:snappy:' + project.'snappy-java.version') {
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java b/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
index aae7d8a136..0e500a33ec 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
@@ -1860,7 +1860,10 @@
    * <U>Default</U>: <code>""</code>
    * </p>
    * <U>Since</U>: GemFire 8.0
+   *
+   * @deprecated Since Geode 1.4 (See GEODE-3007)
    */
+  @Deprecated()
   String USER_COMMAND_PACKAGES = "user-command-packages";
   /**
    * The static String definition of the <i>"off-heap-memory-size"</i> property <a
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
index bfc5e1d5d4..3e5681b74c 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
@@ -1018,9 +1018,6 @@ public static Class _getAttributeType(String attName) {
     m.put(GROUPS,
         "A comma separated list of all the groups this member belongs to." + " Defaults to \"\".");
 
-    m.put(USER_COMMAND_PACKAGES,
-        "A comma separated list of the names of the packages containing classes that implement user commands.");
-
     m.put(JMX_MANAGER,
         "If true then this member is willing to be a jmx manager. Defaults to false except on a locator.");
     m.put(JMX_MANAGER_START,
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java
index 958ea15028..bf353ed9ae 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java
@@ -387,34 +387,6 @@
   File DEFAULT_DEPLOY_WORKING_DIR = new File(System.getProperty("user.dir"));
 
   /**
-   * Returns the value of the {@link ConfigurationProperties#USER_COMMAND_PACKAGES} property
-   */
-  @ConfigAttributeGetter(name = USER_COMMAND_PACKAGES)
-  String getUserCommandPackages();
-
-  /**
-   * Sets the system's user command path.
-   * 
-   * @throws IllegalArgumentException if the specified value is not acceptable.
-   * @throws org.apache.geode.UnmodifiableException if this attribute can not be modified.
-   * @throws org.apache.geode.GemFireIOException if the set failure is caused by an error when
-   *         writing to the system's configuration file.
-   */
-  @ConfigAttributeSetter(name = USER_COMMAND_PACKAGES)
-  void setUserCommandPackages(String value);
-
-  /**
-   * The name of the {@link ConfigurationProperties#USER_COMMAND_PACKAGES} property.
-   */
-  @ConfigAttribute(type = String.class)
-  String USER_COMMAND_PACKAGES_NAME = USER_COMMAND_PACKAGES;
-
-  /**
-   * The default value of the {@link ConfigurationProperties#USER_COMMAND_PACKAGES} property
-   */
-  String DEFAULT_USER_COMMAND_PACKAGES = "";
-
-  /**
    * Returns the value of the {@link ConfigurationProperties#LOG_FILE} property
    * 
    * @return <code>null</code> if logging information goes to standard out
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
index abaa83c822..3862b22c20 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
@@ -625,8 +625,6 @@
   private Map<String, ConfigSource> sourceMap =
       Collections.synchronizedMap(new HashMap<String, ConfigSource>());
 
-  protected String userCommandPackages = DEFAULT_USER_COMMAND_PACKAGES;
-
   /**
    * "off-heap-memory-size" with value of "" or "<size>[g|m]"
    */
@@ -759,7 +757,6 @@ public DistributionConfigImpl(DistributionConfig other) {
     this.redisPort = other.getRedisPort();
     this.redisBindAddress = other.getRedisBindAddress();
     this.redisPassword = other.getRedisPassword();
-    this.userCommandPackages = other.getUserCommandPackages();
 
     // following added for 8.0
     this.enableSharedConfiguration = other.getEnableClusterConfiguration();
@@ -1834,10 +1831,6 @@ public int getAsyncMaxQueueSize() {
     return this.asyncMaxQueueSize;
   }
 
-  public String getUserCommandPackages() {
-    return this.userCommandPackages;
-  }
-
   public int getHttpServicePort() {
     return this.httpServicePort;
   }
@@ -1862,10 +1855,6 @@ public void setStartDevRestApi(boolean value) {
     this.startDevRestApi = value;
   }
 
-  public void setUserCommandPackages(String value) {
-    this.userCommandPackages = value;
-  }
-
   public boolean getDeltaPropagation() {
     return this.deltaPropagation;
   }
@@ -3013,9 +3002,8 @@ public boolean equals(final Object o) {
         .append(sslTrustStore, that.sslTrustStore)
         .append(sslTrustStorePassword, that.sslTrustStorePassword)
         .append(locatorSSLAlias, that.locatorSSLAlias).append(sslDefaultAlias, that.sslDefaultAlias)
-        .append(sourceMap, that.sourceMap).append(userCommandPackages, that.userCommandPackages)
-        .append(offHeapMemorySize, that.offHeapMemorySize).append(shiroInit, that.shiroInit)
-        .isEquals();
+        .append(sourceMap, that.sourceMap).append(offHeapMemorySize, that.offHeapMemorySize)
+        .append(shiroInit, that.shiroInit).isEquals();
   }
 
   /**
@@ -3083,9 +3071,8 @@ public int hashCode() {
         .append(sslCiphers).append(sslRequireAuthentication).append(sslKeyStore)
         .append(sslKeyStoreType).append(sslKeyStorePassword).append(sslTrustStore)
         .append(sslTrustStorePassword).append(sslWebServiceRequireAuthentication)
-        .append(locatorSSLAlias).append(sslDefaultAlias).append(sourceMap)
-        .append(userCommandPackages).append(offHeapMemorySize).append(lockMemory).append(shiroInit)
-        .append(modifiable).toHashCode();
+        .append(locatorSSLAlias).append(sslDefaultAlias).append(sourceMap).append(offHeapMemorySize)
+        .append(lockMemory).append(shiroInit).append(modifiable).toHashCode();
   }
 
   /**
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java b/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
index 5b911d5636..1d65ec949e 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
@@ -338,8 +338,7 @@ public MemberMBeanBridge(InternalCache cache, SystemManagementService service) {
 
     this.config = system.getConfig();
     try {
-      this.commandProcessor =
-          new OnlineCommandProcessor(system.getProperties(), cache.getSecurityService());
+      this.commandProcessor = new OnlineCommandProcessor(cache.getSecurityService());
     } catch (Exception e) {
       commandServiceInitError = e.getMessage();
       logger.info(LogMarker.CONFIG, "Command processor could not be initialized. {}",
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
index 01605986c0..6c23201e94 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
@@ -14,293 +14,96 @@
  */
 package org.apache.geode.management.internal.cli;
 
-import static org.apache.geode.distributed.ConfigurationProperties.USER_COMMAND_PACKAGES;
 
-import java.lang.reflect.Method;
-import java.util.ArrayList;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Properties;
-import java.util.ServiceConfigurationError;
-import java.util.ServiceLoader;
+import static java.util.stream.Collectors.toSet;
+
+import java.util.Objects;
 import java.util.Set;
-import java.util.StringTokenizer;
+import java.util.function.Predicate;
 
+import com.google.common.collect.ImmutableSet;
+import org.springframework.shell.commands.ConsoleCommands;
+import org.springframework.shell.commands.ExitCommands;
+import org.springframework.shell.commands.HelpCommands;
 import org.springframework.shell.converters.EnumConverter;
 import org.springframework.shell.converters.SimpleFileConverter;
 import org.springframework.shell.core.CommandMarker;
 import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
-import org.springframework.shell.core.annotation.CliCommand;
 
-import org.apache.geode.distributed.ConfigurationProperties;
-import org.apache.geode.distributed.internal.DistributionConfig;
-import org.apache.geode.internal.ClassPathLoader;
-import org.apache.geode.management.internal.cli.commands.GfshCommand;
 import org.apache.geode.management.internal.cli.help.Helper;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 import org.apache.geode.management.internal.cli.util.ClasspathScanLoadHelper;
 
 /**
- *
  * this only takes care of loading all available command markers and converters from the application
  * 
  * @since GemFire 7.0
  */
 public class CommandManager {
-  public static final String USER_CMD_PACKAGES_PROPERTY =
-      DistributionConfig.GEMFIRE_PREFIX + USER_COMMAND_PACKAGES;
-  public static final String USER_CMD_PACKAGES_ENV_VARIABLE = "GEMFIRE_USER_COMMAND_PACKAGES";
-  private static final Object INSTANCE_LOCK = new Object();
-
-  private final Helper helper = new Helper();
-
-  private final List<Converter<?>> converters = new ArrayList<Converter<?>>();
-  private final List<CommandMarker> commandMarkers = new ArrayList<>();
+  // Skip some of the Converters from Spring Shell for our customization
+  private static final Set<Class> EXCLUDED_CLASSES = ImmutableSet.of(SimpleFileConverter.class,
+      EnumConverter.class, ExitCommands.class, HelpCommands.class, ConsoleCommands.class);
 
-  private Properties cacheProperties;
-  private LogWrapper logWrapper;
+  private final LogWrapper logWrapper = LogWrapper.getInstance();
+  private final Set<CommandMarker> commandMarkers;
+  private final Set<Converter> converters;
+  private final Helper helper;
 
-  /**
-   * this constructor is used from Gfsh VM. We are getting the user-command-package from system
-   * environment. used by Gfsh.
-   */
   public CommandManager() {
-    this(null);
+    helper = new Helper();
+    converters = loadConverters();
+    commandMarkers = loadCommandMarkers();
   }
 
-  /**
-   * this is used when getting the instance in a cache server. We are getting the
-   * user-command-package from distribution properties. used by OnlineCommandProcessor.
-   */
-  public CommandManager(final Properties cacheProperties) {
-    if (cacheProperties != null) {
-      this.cacheProperties = cacheProperties;
-    }
-    logWrapper = LogWrapper.getInstance();
-    loadCommands();
-  }
+  private Set<Converter> loadConverters() {
+    Set<Converter> converters = instantiateAllClassesImplementing(Converter.class);
+    raiseExceptionIfEmpty(converters, "converters");
 
-  private static void raiseExceptionIfEmpty(Set<Class<?>> foundClasses, String errorFor)
-      throws IllegalStateException {
-    if (foundClasses == null || foundClasses.isEmpty()) {
-      throw new IllegalStateException(
-          "Required " + errorFor + " classes were not loaded. Check logs for errors.");
-    }
+    converters.forEach(this::setContextIfCommandManagerAware);
+    return converters;
   }
 
-  private void loadUserCommands() {
-    final Set<String> userCommandPackages = new HashSet<String>();
-
-    // Find by packages specified by the system property
-    if (System.getProperty(USER_CMD_PACKAGES_PROPERTY) != null) {
-      StringTokenizer tokenizer =
-          new StringTokenizer(System.getProperty(USER_CMD_PACKAGES_PROPERTY), ",");
-      while (tokenizer.hasMoreTokens()) {
-        userCommandPackages.add(tokenizer.nextToken());
-      }
-    }
-
-    // Find by packages specified by the environment variable
-    if (System.getenv().containsKey(USER_CMD_PACKAGES_ENV_VARIABLE)) {
-      StringTokenizer tokenizer =
-          new StringTokenizer(System.getenv().get(USER_CMD_PACKAGES_ENV_VARIABLE), ",");
-      while (tokenizer.hasMoreTokens()) {
-        userCommandPackages.add(tokenizer.nextToken());
-      }
-    }
-
-    // Find by packages specified in the distribution config
-    if (this.cacheProperties != null) {
-      String cacheUserCmdPackages =
-          this.cacheProperties.getProperty(ConfigurationProperties.USER_COMMAND_PACKAGES);
-      if (cacheUserCmdPackages != null && !cacheUserCmdPackages.isEmpty()) {
-        StringTokenizer tokenizer = new StringTokenizer(cacheUserCmdPackages, ",");
-        while (tokenizer.hasMoreTokens()) {
-          userCommandPackages.add(tokenizer.nextToken());
-        }
-      }
-    }
+  private Set<CommandMarker> loadCommandMarkers() {
+    Set<CommandMarker> commandMarkers = instantiateAllClassesImplementing(CommandMarker.class);
+    raiseExceptionIfEmpty(commandMarkers, "commandMarkers");
 
-    // Load commands found in all of the packages
-    for (String userCommandPackage : userCommandPackages) {
-      try {
-        Set<Class<?>> foundClasses = ClasspathScanLoadHelper
-            .scanPackageForClassesImplementing(userCommandPackage, CommandMarker.class);
-        for (Class<?> klass : foundClasses) {
-          try {
-            add((CommandMarker) klass.newInstance());
-          } catch (Exception e) {
-            logWrapper.warning("Could not load User Commands from: " + klass + " due to "
-                + e.getLocalizedMessage()); // continue
-          }
-        }
-        raiseExceptionIfEmpty(foundClasses, "User Command");
-      } catch (IllegalStateException e) {
-        logWrapper.warning(e.getMessage(), e);
-        throw e;
-      }
-    }
-  }
-
-  /**
-   * Loads commands via {@link ServiceLoader} from {@link ClassPathLoader}.
-   *
-   * @since GemFire 8.1
-   */
-  private void loadPluginCommands() {
-    final Iterator<CommandMarker> iterator = ServiceLoader
-        .load(CommandMarker.class, ClassPathLoader.getLatest().asClassLoader()).iterator();
-    while (iterator.hasNext()) {
-      try {
-        final CommandMarker commandMarker = iterator.next();
-        try {
-          add(commandMarker);
-        } catch (Exception e) {
-          logWrapper.warning("Could not load Command from: " + commandMarker.getClass() + " due to "
-              + e.getLocalizedMessage(), e); // continue
-        }
-      } catch (ServiceConfigurationError e) {
-        logWrapper.severe("Could not load Command: " + e.getLocalizedMessage(), e); // continue
-      }
-    }
+    commandMarkers.forEach(this::setContextIfCommandManagerAware);
+    commandMarkers.forEach(helper::registerCommand);
+    return commandMarkers;
   }
 
+  private <T> Set<T> instantiateAllClassesImplementing(Class<T> implementedInterface) {
+    Set<Class<? extends T>> classes =
+        ClasspathScanLoadHelper.scanClasspathForClassesImplementing(implementedInterface);
 
-  private void loadCommands() {
-    loadUserCommands();
+    Predicate<Class<? extends T>> classIsNotExcluded = aClass -> !EXCLUDED_CLASSES.contains(aClass);
 
-    loadPluginCommands();
-    loadGeodeCommands();
-    loadConverters();
+    return classes.stream().filter(classIsNotExcluded).map(this::instantiateClass)
+        .filter(Objects::nonNull).collect(toSet());
   }
 
-  private void loadConverters() {
-    Set<Class<?>> foundClasses;
-    // Converters
+  private <T> T instantiateClass(Class<T> classToInstantiate) {
     try {
-      foundClasses = ClasspathScanLoadHelper.scanPackageForClassesImplementing(
-          "org.apache.geode.management.internal.cli.converters", Converter.class);
-      for (Class<?> klass : foundClasses) {
-        try {
-          Converter<?> object = (Converter<?>) klass.newInstance();
-          add(object);
-
-        } catch (Exception e) {
-          logWrapper.warning(
-              "Could not load Converter from: " + klass + " due to " + e.getLocalizedMessage()); // continue
-        }
-      }
-      raiseExceptionIfEmpty(foundClasses, "Converters");
-
-      // Spring shell's converters
-      foundClasses = ClasspathScanLoadHelper.scanPackageForClassesImplementing(
-          "org.springframework.shell.converters", Converter.class);
-      for (Class<?> klass : foundClasses) {
-        if (!SHL_CONVERTERS_TOSKIP.contains(klass)) {
-          try {
-            add((Converter<?>) klass.newInstance());
-          } catch (Exception e) {
-            logWrapper.warning(
-                "Could not load Converter from: " + klass + " due to " + e.getLocalizedMessage()); // continue
-          }
-        }
-      }
-      raiseExceptionIfEmpty(foundClasses, "Basic Converters");
-    } catch (IllegalStateException e) {
-      logWrapper.warning(e.getMessage(), e);
-      throw e;
+      return classToInstantiate.newInstance();
+    } catch (Exception e) {
+      logWrapper.warning("Could not load command or converter from: " + classToInstantiate, e);
     }
+    return null;
   }
 
-  private void loadGeodeCommands() {
-    // CommandMarkers
-    Set<Class<?>> foundClasses;
-    try {
-      // geode's commands
-      foundClasses = ClasspathScanLoadHelper.scanPackageForClassesImplementing(
-          GfshCommand.class.getPackage().getName(), CommandMarker.class);
-
-      for (Class<?> klass : foundClasses) {
-        try {
-          add((CommandMarker) klass.newInstance());
-        } catch (Exception e) {
-          logWrapper.warning(
-              "Could not load Command from: " + klass + " due to " + e.getLocalizedMessage()); // continue
-        }
-      }
-      raiseExceptionIfEmpty(foundClasses, "Commands");
-
-      // do not add Spring shell's commands for now. When we add it, we need to tell the parser that
-      // these are offline commands.
-    } catch (IllegalStateException e) {
-      logWrapper.warning(e.getMessage(), e);
-      throw e;
+  private void setContextIfCommandManagerAware(Object commandOrConverter) {
+    if (CommandManagerAware.class.isAssignableFrom(commandOrConverter.getClass())) {
+      ((CommandManagerAware) commandOrConverter).setCommandManager(this);
     }
   }
 
-  /** Skip some of the Converters from Spring Shell for our customization */
-  private static List<Class> SHL_CONVERTERS_TOSKIP = new ArrayList();
-  static {
-    // skip springs SimpleFileConverter to use our own FilePathConverter
-    SHL_CONVERTERS_TOSKIP.add(SimpleFileConverter.class);
-    // skip spring's EnumConverter to use our own EnumConverter
-    SHL_CONVERTERS_TOSKIP.add(EnumConverter.class);
-  }
-
-  public List<Converter<?>> getConverters() {
-    return converters;
-  }
-
-  public List<CommandMarker> getCommandMarkers() {
-    return commandMarkers;
-  }
-
-  /**
-   * Method to add new Converter
-   *
-   * @param converter
-   */
-  void add(Converter<?> converter) {
-    if (CommandManagerAware.class.isAssignableFrom(converter.getClass())) {
-      ((CommandManagerAware) converter).setCommandManager(this);
-    }
-    converters.add(converter);
-  }
-
-  /**
-   * Method to add new Commands to the parser
-   *
-   * @param commandMarker
-   */
-  void add(CommandMarker commandMarker) {
-    if (CommandManagerAware.class.isAssignableFrom(commandMarker.getClass())) {
-      ((CommandManagerAware) commandMarker).setCommandManager(this);
-    }
-    commandMarkers.add(commandMarker);
-    for (Method method : commandMarker.getClass().getMethods()) {
-      CliCommand cliCommand = method.getAnnotation(CliCommand.class);
-      CliAvailabilityIndicator availability = method.getAnnotation(CliAvailabilityIndicator.class);
-      if (cliCommand == null && availability == null) {
-        continue;
-      }
-
-      if (cliCommand != null) {
-        helper.addCommand(cliCommand, method);
-      }
-
-      if (availability != null) {
-        helper.addAvailabilityIndicator(availability, new MethodTarget(method, commandMarker));
-      }
+  private static void raiseExceptionIfEmpty(Set<?> foundClasses, String classType)
+      throws IllegalStateException {
+    if (foundClasses == null || foundClasses.isEmpty()) {
+      throw new IllegalStateException("No " + classType + " were loaded. Check logs for errors.");
     }
   }
 
-  public Helper getHelper() {
-    return helper;
-  }
-
   public String obtainHelp(String buffer) {
     int terminalWidth = -1;
     Gfsh gfsh = Gfsh.getCurrentInstance();
@@ -314,4 +117,15 @@ public String obtainHint(String topic) {
     return helper.getHint(topic);
   }
 
+  public Set<Converter> getConverters() {
+    return converters;
+  }
+
+  public Set<CommandMarker> getCommandMarkers() {
+    return commandMarkers;
+  }
+
+  public Helper getHelper() {
+    return helper;
+  }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
index 9cebc63c34..179d4ec44b 100755
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
@@ -57,10 +57,14 @@ public GfshParser(CommandManager commandManager) {
       add(command);
     }
 
-    for (Converter<?> converter : commandManager.getConverters()) {
+    for (Converter converter : commandManager.getConverters()) {
       if (converter.getClass().isAssignableFrom(ArrayConverter.class)) {
         ArrayConverter arrayConverter = (ArrayConverter) converter;
-        arrayConverter.setConverters(new HashSet<>(commandManager.getConverters()));
+        HashSet<Converter<?>> converters = new HashSet<>();
+        for (Converter c : commandManager.getConverters()) {
+          converters.add(c);
+        }
+        arrayConverter.setConverters(converters);
       }
       add(converter);
     }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
index 352501324c..a712e37f62 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
@@ -14,18 +14,10 @@
  */
 package org.apache.geode.management.internal.cli.help;
 
-import org.apache.commons.lang.StringUtils;
-import org.apache.geode.management.cli.CliMetaData;
-import org.apache.geode.management.internal.cli.GfshParser;
-import org.apache.geode.management.internal.cli.i18n.CliStrings;
-import org.springframework.shell.core.MethodTarget;
-import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
-import org.springframework.shell.core.annotation.CliCommand;
-import org.springframework.shell.core.annotation.CliOption;
+import static java.util.stream.Collectors.joining;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -35,6 +27,17 @@
 import java.util.Set;
 import java.util.TreeMap;
 
+import org.apache.commons.lang.StringUtils;
+import org.springframework.shell.core.CommandMarker;
+import org.springframework.shell.core.MethodTarget;
+import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.internal.cli.GfshParser;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+
 /**
  * 
  * 
@@ -93,7 +96,25 @@ private void initTopic(String topic, String desc) {
     topics.put(topic, new Topic(topic, desc));
   }
 
-  public void addCommand(CliCommand command, Method commandMethod) {
+  public void registerCommand(CommandMarker commandMarker) {
+    for (Method method : commandMarker.getClass().getMethods()) {
+      CliCommand cliCommand = method.getAnnotation(CliCommand.class);
+      CliAvailabilityIndicator availability = method.getAnnotation(CliAvailabilityIndicator.class);
+      if (cliCommand == null && availability == null) {
+        continue;
+      }
+
+      if (cliCommand != null) {
+        addCommand(cliCommand, method);
+      }
+
+      if (availability != null) {
+        addAvailabilityIndicator(availability, new MethodTarget(method, commandMarker));
+      }
+    }
+  }
+
+  protected void addCommand(CliCommand command, Method commandMethod) {
     // put all the command synonyms in the command map
     Arrays.stream(command.value()).forEach(cmd -> {
       commands.put(cmd, commandMethod);
@@ -119,7 +140,8 @@ public void addCommand(CliCommand command, Method commandMethod) {
     });
   }
 
-  public void addAvailabilityIndicator(CliAvailabilityIndicator availability, MethodTarget target) {
+  private void addAvailabilityIndicator(CliAvailabilityIndicator availability,
+      MethodTarget target) {
     Arrays.stream(availability.value()).forEach(command -> {
       availabilityIndicators.put(command, target);
     });
@@ -151,9 +173,7 @@ public String getHint(String buffer) {
       builder.append(CliStrings.HINT__MSG__TOPICS_AVAILABLE).append(GfshParser.LINE_SEPARATOR)
           .append(GfshParser.LINE_SEPARATOR);
 
-      List<String> sortedTopics = new ArrayList<>(topics.keySet());
-      Collections.sort(sortedTopics);
-      sortedTopics.stream()
+      topics.keySet().stream().sorted()
           .forEachOrdered(topic -> builder.append(topic).append(GfshParser.LINE_SEPARATOR));
       return builder.toString();
     }
@@ -164,8 +184,7 @@ public String getHint(String buffer) {
     }
 
     builder.append(topic.desc).append(GfshParser.LINE_SEPARATOR).append(GfshParser.LINE_SEPARATOR);
-    Collections.sort(topic.relatedCommands);
-    topic.relatedCommands.stream().forEachOrdered(command -> builder.append(command.command)
+    topic.relatedCommands.stream().sorted().forEach(command -> builder.append(command.command)
         .append(": ").append(command.desc).append(GfshParser.LINE_SEPARATOR));
     return builder.toString();
   }
@@ -174,7 +193,7 @@ public String getHint(String buffer) {
     return topics.keySet();
   }
 
-  boolean isAvailable(String command) {
+  private boolean isAvailable(String command) {
     MethodTarget target = availabilityIndicators.get(command);
     if (target == null) {
       return true;
@@ -260,16 +279,12 @@ HelpBlock getOptionDetail(CliOption cliOption) {
     HelpBlock optionNode = new HelpBlock(getPrimaryKey(cliOption));
     String help = cliOption.help();
     optionNode.addChild(new HelpBlock((StringUtils.isNotBlank(help) ? help : "")));
-    if (getSynonyms(cliOption).size() > 0) {
-      StringBuilder builder = new StringBuilder();
-      for (String string : getSynonyms(cliOption)) {
-        if (builder.length() > 0) {
-          builder.append(",");
-        }
-        builder.append(string);
-      }
-      optionNode.addChild(new HelpBlock(SYNONYMS_SUB_NAME + builder.toString()));
+
+    String synonyms = getSynonyms(cliOption).stream().collect(joining(","));
+    if (StringUtils.isNotEmpty(synonyms)) {
+      optionNode.addChild(new HelpBlock(SYNONYMS_SUB_NAME + synonyms));
     }
+
     optionNode.addChild(
         new HelpBlock(REQUIRED_SUB_NAME + ((cliOption.mandatory()) ? TRUE_TOKEN : FALSE_TOKEN)));
     if (!isNullOrBlank(cliOption.specifiedDefaultValue())) {
@@ -358,18 +373,14 @@ private static String getPrimaryKey(CliOption option) {
   }
 
   private static List<String> getSynonyms(CliOption option) {
-    List<String> synonyms = new ArrayList<>();
     String[] keys = option.key();
     if (keys.length < 2)
-      return synonyms;
+      return Collections.emptyList();
     // if the primary key is empty (like sh and help command), then there should be no synonyms.
     if ("".equals(keys[0]))
-      return synonyms;
+      return Collections.emptyList();
 
-    for (int i = 1; i < keys.length; i++) {
-      synonyms.add(keys[i]);
-    }
-    return synonyms;
+    return Arrays.asList(keys).subList(1, keys.length);
   }
 
   private static boolean isNullOrBlank(String value) {
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/MemberCommandService.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/MemberCommandService.java
index 6130117d2e..a4c6f59802 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/MemberCommandService.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/MemberCommandService.java
@@ -35,8 +35,7 @@
   public MemberCommandService(InternalCache cache) throws CommandServiceException {
     this.cache = cache;
     try {
-      this.onlineCommandProcessor = new OnlineCommandProcessor(
-          cache.getDistributedSystem().getProperties(), cache.getSecurityService());
+      this.onlineCommandProcessor = new OnlineCommandProcessor(cache.getSecurityService());
     } catch (Exception e) {
       throw new CommandServiceException("Could not load commands.", e);
     }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java
index 7fa4acba51..38ab364392 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java
@@ -18,7 +18,6 @@
 import java.lang.reflect.Method;
 import java.util.Collections;
 import java.util.Map;
-import java.util.Properties;
 
 import org.springframework.shell.core.Parser;
 import org.springframework.shell.event.ParseResult;
@@ -26,7 +25,6 @@
 
 import org.apache.geode.annotations.TestingOnly;
 import org.apache.geode.internal.security.SecurityService;
-import org.apache.geode.internal.security.SecurityServiceFactory;
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.CommandProcessingException;
 import org.apache.geode.management.cli.Result;
@@ -49,20 +47,14 @@
 
   private final SecurityService securityService;
 
-  @TestingOnly
-  public OnlineCommandProcessor() throws ClassNotFoundException, IOException {
-    this(new Properties(), SecurityServiceFactory.create());
-  }
-
-  public OnlineCommandProcessor(Properties cacheProperties, SecurityService securityService)
+  public OnlineCommandProcessor(SecurityService securityService)
       throws ClassNotFoundException, IOException {
-    this(cacheProperties, securityService, new CommandExecutor());
+    this(securityService, new CommandExecutor());
   }
 
   @TestingOnly
-  public OnlineCommandProcessor(Properties cacheProperties, SecurityService securityService,
-      CommandExecutor commandExecutor) {
-    this.gfshParser = new GfshParser(new CommandManager(cacheProperties));
+  public OnlineCommandProcessor(SecurityService securityService, CommandExecutor commandExecutor) {
+    this.gfshParser = new GfshParser(new CommandManager());
     this.executor = commandExecutor;
     this.securityService = securityService;
   }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
index d3bc7d3094..3713d5a9ab 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java
@@ -16,30 +16,39 @@
 
 import static java.util.stream.Collectors.toSet;
 
-import io.github.lukehutch.fastclasspathscanner.FastClasspathScanner;
-
 import java.lang.reflect.Modifier;
 import java.util.HashSet;
 import java.util.Set;
 
+import io.github.lukehutch.fastclasspathscanner.FastClasspathScanner;
+import io.github.lukehutch.fastclasspathscanner.matchprocessor.ImplementingClassMatchProcessor;
+
 /**
  * Utility class to scan class-path & load classes.
  * 
  * @since GemFire 7.0
  */
 public class ClasspathScanLoadHelper {
-  public static Set<Class<?>> scanPackageForClassesImplementing(String packageToScan,
-      Class<?> implementedInterface) {
-    Set<Class<?>> classesImplementing = new HashSet<>();
+
+  public static <T> Set<Class<? extends T>> scanClasspathForClassesImplementing(
+      Class<T> implementedInterface) {
+    return scanPackageForClassesImplementing("", implementedInterface);
+  }
+
+  public static <T> Set<Class<? extends T>> scanPackageForClassesImplementing(String packageToScan,
+      Class<T> implementedInterface) {
+    Set<Class<? extends T>> classesImplementing = new HashSet<>();
+    ImplementingClassMatchProcessor<T> matchProcessor = classesImplementing::add;
+
     new FastClasspathScanner(packageToScan)
-        .matchClassesImplementing(implementedInterface, classesImplementing::add).scan();
+        .matchClassesImplementing(implementedInterface, matchProcessor).scan();
 
     return classesImplementing.stream().filter(ClasspathScanLoadHelper::isInstantiable)
         .collect(toSet());
   }
 
-  private static boolean isInstantiable(Class<?> klass) {
-    int modifiers = klass.getModifiers();
+  private static <T> boolean isInstantiable(Class<T> classToInstantiate) {
+    int modifiers = classToInstantiate.getModifiers();
 
     return !Modifier.isAbstract(modifiers) && !Modifier.isInterface(modifiers)
         && Modifier.isPublic(modifiers);
diff --git a/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java b/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
index 7e5f83f4f2..e874898d82 100644
--- a/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
@@ -99,7 +99,7 @@ public void before() {
   @Test
   public void testGetAttributeNames() {
     String[] attNames = AbstractDistributionConfig._getAttNames();
-    assertEquals(attNames.length, 157);
+    assertEquals(156, attNames.length);
 
     List boolList = new ArrayList();
     List intList = new ArrayList();
@@ -135,7 +135,7 @@ public void testGetAttributeNames() {
     // are.
     assertEquals(29, boolList.size());
     assertEquals(33, intList.size());
-    assertEquals(86, stringList.size());
+    assertEquals(85, stringList.size());
     assertEquals(5, fileList.size());
     assertEquals(4, otherList.size());
   }
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
index 827eb9c400..04794d4559 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java
@@ -14,11 +14,11 @@
  */
 package org.apache.geode.management.internal.cli;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import java.util.Set;
 
-import org.apache.geode.management.internal.cli.util.ClasspathScanLoadHelper;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
@@ -28,6 +28,7 @@
 import org.apache.geode.management.internal.cli.domain.Impl12;
 import org.apache.geode.management.internal.cli.domain.Interface1;
 import org.apache.geode.management.internal.cli.domain.Interface2;
+import org.apache.geode.management.internal.cli.util.ClasspathScanLoadHelper;
 import org.apache.geode.test.junit.categories.UnitTest;
 
 @Category(UnitTest.class)
@@ -35,12 +36,12 @@
 
   private final String PACKAGE_NAME = "org.apache.geode.management.internal.cli.domain";
   private final String WRONG_PACKAGE_NAME = "org.apache.geode.management.internal.cli.domain1";
-  private final Class<?> INTERFACE1 = Interface1.class;
-  private final Class<?> NO_IMPL_INTERFACE = Versionable.class;
-  private final Class<?> INTERFACE2 = Interface2.class;
-  private final Class<?> IMPL1 = Impl1.class;
-  private final Class<?> IMPL2 = Impl12.class;
-  private final Class<?> ABSTRACT_IMPL = AbstractImpl.class;
+  private final Class INTERFACE1 = Interface1.class;
+  private final Class NO_IMPL_INTERFACE = Versionable.class;
+  private final Class INTERFACE2 = Interface2.class;
+  private final Class IMPL1 = Impl1.class;
+  private final Class IMPL2 = Impl12.class;
+  private final Class ABSTRACT_IMPL = AbstractImpl.class;
 
   @Test
   public void testLoadAndGet() throws Exception {
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java
index cde0b236a7..9c3ebf57a8 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/CommandManagerJUnitTest.java
@@ -18,20 +18,12 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
-import java.util.List;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.springframework.shell.core.CommandMarker;
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
 import org.springframework.shell.core.annotation.CliCommand;
-import org.springframework.shell.core.annotation.CliOption;
 
-import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.Result;
 import org.apache.geode.management.internal.security.ResourceOperation;
 import org.apache.geode.security.ResourcePermission.Operation;
@@ -43,49 +35,6 @@
  */
 @Category(UnitTest.class)
 public class CommandManagerJUnitTest {
-
-  private static final String COMMAND1_NAME = "command1";
-  private static final String COMMAND1_NAME_ALIAS = "command1_alias";
-  private static final String COMMAND2_NAME = "c2";
-
-  private static final String COMMAND1_HELP = "help for " + COMMAND1_NAME;
-  // ARGUMENTS
-  private static final String ARGUMENT1_NAME = "argument1";
-  private static final String ARGUMENT1_HELP = "help for argument1";
-  private static final String ARGUMENT1_CONTEXT = "context for argument 1";
-  private static final Completion[] ARGUMENT1_COMPLETIONS =
-      {new Completion("arg1"), new Completion("arg1alt")};
-  private static final String ARGUMENT2_NAME = "argument2";
-  private static final String ARGUMENT2_CONTEXT = "context for argument 2";
-  private static final String ARGUMENT2_HELP = "help for argument2";
-  private static final String ARGUMENT2_UNSPECIFIED_DEFAULT_VALUE =
-      "{unspecified default value for argument2}";
-  private static final Completion[] ARGUMENT2_COMPLETIONS =
-      {new Completion("arg2"), new Completion("arg2alt")};
-
-  // OPTIONS
-  private static final String OPTION1_NAME = "option1";
-  private static final String OPTION1_SYNONYM = "opt1";
-  private static final String OPTION1_HELP = "help for option1";
-  private static final String OPTION1_CONTEXT = "context for option1";
-  private static final String OPTION1_SPECIFIED_DEFAULT_VALUE =
-      "{specified default value for option1}";
-  private static final Completion[] OPTION1_COMPLETIONS =
-      {new Completion("option1"), new Completion("option1Alternate")};
-  private static final String OPTION2_NAME = "option2";
-  private static final String OPTION2_HELP = "help for option2";
-  private static final String OPTION2_CONTEXT = "context for option2";
-  private static final String OPTION2_SPECIFIED_DEFAULT_VALUE =
-      "{specified default value for option2}";
-  private static final String OPTION3_NAME = "option3";
-  private static final String OPTION3_SYNONYM = "opt3";
-  private static final String OPTION3_HELP = "help for option3";
-  private static final String OPTION3_CONTEXT = "context for option3";
-  private static final String OPTION3_SPECIFIED_DEFAULT_VALUE =
-      "{specified default value for option3}";
-  private static final String OPTION3_UNSPECIFIED_DEFAULT_VALUE =
-      "{unspecified default value for option3}";
-
   private CommandManager commandManager;
 
   @Before
@@ -111,11 +60,7 @@ public void testCommandManagerInstance() throws Exception {
     assertNotNull(commandManager);
   }
 
-  /**
-   * Tests {@link CommandManager#loadPluginCommands()}.
-   * 
-   * @since GemFire 8.1
-   */
+
   @Test
   public void testCommandManagerLoadPluginCommands() throws Exception {
     assertNotNull(commandManager);
@@ -123,97 +68,10 @@ public void testCommandManagerLoadPluginCommands() throws Exception {
     // see META-INF/services/org.springframework.shell.core.CommandMarker service loader file.
     assertTrue("Should find listed plugin.",
         commandManager.getHelper().getCommands().contains("mock plugin command"));
-    assertTrue("Should not find unlisted plugin.",
-        !commandManager.getHelper().getCommands().contains("mock plugin command unlisted"));
+    assertThat(
+        commandManager.getCommandMarkers().stream().anyMatch(c -> c instanceof MockPluginCommand));
   }
 
-  /**
-   * class that represents dummy commands
-   */
-  public static class Commands implements CommandMarker {
-
-    @CliCommand(value = {COMMAND1_NAME, COMMAND1_NAME_ALIAS}, help = COMMAND1_HELP)
-    @CliMetaData(shellOnly = true, relatedTopic = {"relatedTopicOfCommand1"})
-    @ResourceOperation(resource = Resource.CLUSTER, operation = Operation.READ)
-    public static String command1(
-        @CliOption(key = ARGUMENT1_NAME, optionContext = ARGUMENT1_CONTEXT, help = ARGUMENT1_HELP,
-            mandatory = true) String argument1,
-        @CliOption(key = ARGUMENT2_NAME, optionContext = ARGUMENT2_CONTEXT, help = ARGUMENT2_HELP,
-            unspecifiedDefaultValue = ARGUMENT2_UNSPECIFIED_DEFAULT_VALUE) String argument2,
-        @CliOption(key = {OPTION1_NAME, OPTION1_SYNONYM}, help = OPTION1_HELP, mandatory = true,
-            optionContext = OPTION1_CONTEXT,
-            specifiedDefaultValue = OPTION1_SPECIFIED_DEFAULT_VALUE) String option1,
-        @CliOption(key = {OPTION2_NAME}, help = OPTION2_HELP, optionContext = OPTION2_CONTEXT,
-            specifiedDefaultValue = OPTION2_SPECIFIED_DEFAULT_VALUE) String option2,
-        @CliOption(key = {OPTION3_NAME, OPTION3_SYNONYM}, help = OPTION3_HELP,
-            optionContext = OPTION3_CONTEXT,
-            unspecifiedDefaultValue = OPTION3_UNSPECIFIED_DEFAULT_VALUE,
-            specifiedDefaultValue = OPTION3_SPECIFIED_DEFAULT_VALUE) String option3) {
-      return null;
-    }
-
-    @CliCommand(value = {COMMAND2_NAME})
-    @ResourceOperation(resource = Resource.CLUSTER, operation = Operation.READ)
-    public static String command2() {
-      return null;
-    }
-
-    @CliCommand(value = {"testParamConcat"})
-    @ResourceOperation(resource = Resource.CLUSTER, operation = Operation.READ)
-    public static Result testParamConcat(@CliOption(key = {"string"}) String string,
-        @CliOption(key = {"stringArray"}) String[] stringArray,
-        @CliOption(key = {"integer"}) Integer integer,
-        @CliOption(key = {"colonArray"}) String[] colonArray) {
-      return null;
-    }
-
-    @CliCommand(value = {"testMultiWordArg"})
-    @ResourceOperation(resource = Resource.CLUSTER, operation = Operation.READ)
-    public static Result testMultiWordArg(@CliOption(key = "arg1") String arg1,
-        @CliOption(key = "arg2") String arg2) {
-      return null;
-    }
-
-    @CliAvailabilityIndicator({COMMAND1_NAME})
-    public boolean isAvailable() {
-      return true; // always available on server
-    }
-  }
-
-  /**
-   * Used by testCommandManagerLoadPluginCommands
-   */
-  private static class SimpleConverter implements Converter<String> {
-
-    @Override
-    public boolean supports(Class<?> type, String optionContext) {
-      return type.isAssignableFrom(String.class);
-    }
-
-    @Override
-    public String convertFromText(String value, Class<?> targetType, String optionContext) {
-      return value;
-    }
-
-    @Override
-    public boolean getAllPossibleValues(List<Completion> completions, Class<?> targetType,
-        String existingData, String context, MethodTarget target) {
-      if (context.equals(ARGUMENT1_CONTEXT)) {
-        for (Completion completion : ARGUMENT1_COMPLETIONS) {
-          completions.add(completion);
-        }
-      } else if (context.equals(ARGUMENT2_CONTEXT)) {
-        for (Completion completion : ARGUMENT2_COMPLETIONS) {
-          completions.add(completion);
-        }
-      } else if (context.equals(OPTION1_CONTEXT)) {
-        for (Completion completion : OPTION1_COMPLETIONS) {
-          completions.add(completion);
-        }
-      }
-      return true;
-    }
-  }
 
   public static class MockPluginCommand implements CommandMarker {
     @CliCommand(value = "mock plugin command")
@@ -222,13 +80,4 @@ public Result mockPluginCommand() {
       return null;
     }
   }
-
-  public static class MockPluginCommandUnlisted implements CommandMarker {
-    @CliCommand(value = "mock plugin command unlisted")
-    @ResourceOperation(resource = Resource.CLUSTER, operation = Operation.READ)
-    public Result mockPluginCommandUnlisted() {
-      return null;
-    }
-  }
-
 }
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java
index 20314faa9f..d79993201b 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java
@@ -23,6 +23,19 @@
 import static org.apache.geode.test.dunit.LogWriterUtils.getLogWriter;
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.io.IOException;
+import java.io.PrintStream;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.junit.Rule;
+import org.springframework.shell.core.CommandMarker;
+
 import org.apache.geode.cache.Cache;
 import org.apache.geode.internal.AvailablePortHelper;
 import org.apache.geode.management.ManagementService;
@@ -37,18 +50,6 @@
 import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
 import org.apache.geode.test.dunit.rules.DistributedRestoreSystemProperties;
 import org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder;
-import org.junit.Rule;
-import org.springframework.shell.core.CommandMarker;
-
-import java.io.IOException;
-import java.io.PrintStream;
-import java.net.InetAddress;
-import java.net.UnknownHostException;
-import java.util.List;
-import java.util.Map;
-import java.util.Properties;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 /**
  * Base class for all the CLI/gfsh command dunit tests.
@@ -79,7 +80,7 @@
 
   public static boolean checkIfCommandsAreLoadedOrNot() {
     CommandManager manager = new CommandManager();
-    List<CommandMarker> commands = manager.getCommandMarkers();
+    Set<CommandMarker> commands = manager.getCommandMarkers();
     return commands.size() >= 1;
 
   }
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java
index 58d09f09eb..5786e8a4c9 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/help/HelperUnitTest.java
@@ -15,12 +15,14 @@
 
 package org.apache.geode.management.internal.cli.help;
 
+import static org.apache.geode.management.internal.cli.GfshParser.LINE_SEPARATOR;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import static org.apache.geode.management.internal.cli.GfshParser.LINE_SEPARATOR;
-import org.apache.geode.test.junit.categories.UnitTest;
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
@@ -29,8 +31,7 @@
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 
-import java.lang.annotation.Annotation;
-import java.lang.reflect.Method;
+import org.apache.geode.test.junit.categories.UnitTest;
 
 @Category(UnitTest.class)
 public class HelperUnitTest {
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java
index df00cf17aa..db7176ba4f 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessorTest.java
@@ -21,8 +21,6 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import java.util.Properties;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
@@ -36,7 +34,6 @@
 @Category(UnitTest.class)
 public class OnlineCommandProcessorTest {
 
-  Properties properties;
   SecurityService securityService;
   CommandExecutor executor;
   OnlineCommandProcessor onlineCommandProcessor;
@@ -44,13 +41,12 @@
 
   @Before
   public void before() {
-    properties = new Properties();
     securityService = mock(SecurityService.class);
     executor = mock(CommandExecutor.class);
     result = mock(Result.class);
     when(executor.execute(any())).thenReturn(result);
 
-    onlineCommandProcessor = new OnlineCommandProcessor(properties, securityService, executor);
+    onlineCommandProcessor = new OnlineCommandProcessor(securityService, executor);
   }
 
 
diff --git a/geode-core/src/test/resources/META-INF/services/org.springframework.shell.core.CommandMarker b/geode-core/src/test/resources/META-INF/services/org.springframework.shell.core.CommandMarker
deleted file mode 100644
index 1bb7c6d141..0000000000
--- a/geode-core/src/test/resources/META-INF/services/org.springframework.shell.core.CommandMarker
+++ /dev/null
@@ -1,8 +0,0 @@
-# Mock command for org.apache.geode.management.internal.cli.CommandManagerJUnitTest.testLoadPluginCommands
-org.apache.geode.management.internal.cli.CommandManagerJUnitTest$MockPluginCommand
-# Should cause ServiceConfigurationException while iterating.
-#TODO jbarrett - causes tests to be marked as failed because of exception in log org.apache.geode.management.internal.cli.CommandManagerJUnitTest$MockPluginCommandNotFound
-
-
-# Mock Extension commands
-org.apache.geode.internal.cache.extension.mock.MockExtensionCommands
\ No newline at end of file
diff --git a/geode-lucene/src/main/resources/META-INF/services/org.springframework.shell.core.CommandMarker b/geode-lucene/src/main/resources/META-INF/services/org.springframework.shell.core.CommandMarker
deleted file mode 100644
index 0879e15732..0000000000
--- a/geode-lucene/src/main/resources/META-INF/services/org.springframework.shell.core.CommandMarker
+++ /dev/null
@@ -1,2 +0,0 @@
-# Lucene Extensions command
-org.apache.geode.cache.lucene.internal.cli.LuceneIndexCommands
\ No newline at end of file


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Simplify support for custom GFSH commands
> -----------------------------------------
>
>                 Key: GEODE-3007
>                 URL: https://issues.apache.org/jira/browse/GEODE-3007
>             Project: Geode
>          Issue Type: Improvement
>          Components: docs, gfsh
>            Reporter: Jared Stewart
>            Assignee: Jared Stewart
>
> Geode currently supports three ways to load GFSH commands: 
> 1. Scan the classpath for commands in "org.apache.geode.management.internal.cli.commands”
> 2. Scan the classpath for commands in a package specified by a user via the “user-command-packages” system property. 
> 3. Scan the classpath for commands registered in files inside META-INF.services (e.g. "geode-core/src/test/resources/META-INF/services/org.springframework.shell.core.CommandMarker”) 
> After the improvements made by GEODE-2989, there is no reason to require a user to specify the location of their custom commands via one of these mechanisms.  Instead, we should simply scan the entire classpath for any classes implementing CommandMarker (regardless of whatever packages they live in).  



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message