[GitHub] [geode] jinmeiliao commented on a change in pull request #6423: GEODE-9227: Performance improvements in the loading of CommandMarkers…

[GitBox <gi...@apache.org>]

jinmeiliao commented on a change in pull request #6423:
URL: https://github.com/apache/geode/pull/6423#discussion_r625849420



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
##########
@@ -176,16 +177,15 @@ private void loadPluginCommands() {
   private void loadCommands() {
     Set<String> userCommandPackages = getUserCommandPackages();
     Set<String> packagesToScan = new HashSet<>(userCommandPackages);
-    packagesToScan.add("org.apache.geode");
     packagesToScan.add("org.springframework.shell.converters");
-    packagesToScan.add(GfshCommand.class.getPackage().getName());
-    packagesToScan.add(VersionCommand.class.getPackage().getName());
+    // packagesToScan.add(GfshCommand.class.getPackage().getName());
+    // packagesToScan.add(VersionCommand.class.getPackage().getName());
 
     // Create one scanner to be used everywhere
     try (ClasspathScanLoadHelper scanner = new ClasspathScanLoadHelper(packagesToScan)) {
       loadUserCommands(scanner, userCommandPackages);

Review comment:
       we can just pass in the `userCommandsPackages` for this method, and create the scanner inside the method, we don't need all the previous lines.

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
##########
@@ -176,16 +177,15 @@ private void loadPluginCommands() {
   private void loadCommands() {
     Set<String> userCommandPackages = getUserCommandPackages();
     Set<String> packagesToScan = new HashSet<>(userCommandPackages);
-    packagesToScan.add("org.apache.geode");
     packagesToScan.add("org.springframework.shell.converters");
-    packagesToScan.add(GfshCommand.class.getPackage().getName());
-    packagesToScan.add(VersionCommand.class.getPackage().getName());
+    // packagesToScan.add(GfshCommand.class.getPackage().getName());
+    // packagesToScan.add(VersionCommand.class.getPackage().getName());
 
     // Create one scanner to be used everywhere
     try (ClasspathScanLoadHelper scanner = new ClasspathScanLoadHelper(packagesToScan)) {
       loadUserCommands(scanner, userCommandPackages);
       loadPluginCommands();
-      loadGeodeCommands(scanner);
+      // loadGeodeCommands(scanner);
       loadConverters(scanner);

Review comment:
       since we are using service loader to load all the geode converters, we don't need to pass in the scanner here anymore. Inside the `loadConverters` we can create a scanner only for `"org.springframework.shell.converters"` package and add those converters.

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java
##########
@@ -176,16 +177,15 @@ private void loadPluginCommands() {
   private void loadCommands() {
     Set<String> userCommandPackages = getUserCommandPackages();
     Set<String> packagesToScan = new HashSet<>(userCommandPackages);
-    packagesToScan.add("org.apache.geode");
     packagesToScan.add("org.springframework.shell.converters");
-    packagesToScan.add(GfshCommand.class.getPackage().getName());
-    packagesToScan.add(VersionCommand.class.getPackage().getName());
+    // packagesToScan.add(GfshCommand.class.getPackage().getName());
+    // packagesToScan.add(VersionCommand.class.getPackage().getName());
 
     // Create one scanner to be used everywhere
     try (ClasspathScanLoadHelper scanner = new ClasspathScanLoadHelper(packagesToScan)) {
       loadUserCommands(scanner, userCommandPackages);
       loadPluginCommands();

Review comment:
       this code actually loads all geode commands, right? let's rename it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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