You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/02/14 20:11:44 UTC

[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7363: GEODE-10034: Organize Geode For Redis Stats By Category

dschneider-pivotal commented on a change in pull request #7363:
URL: https://github.com/apache/geode/pull/7363#discussion_r806162348



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> completedCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> timeCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
 
-  private static final int currentlyConnectedClients;
+  private static final int currentlyConnectedClientsId;
   private static final int passiveExpirationChecksId;
   private static final int passiveExpirationCheckTimeId;
   private static final int passiveExpirationsId;
   private static final int expirationsId;
   private static final int expirationTimeId;
-  private static final int totalConnectionsReceived;
-  private static final int commandsProcessed;
-  private static final int keyspaceHits;
-  private static final int keyspaceMisses;
-  private static final int totalNetworkBytesRead;
+  private static final int totalConnectionsReceivedId;
+  private static final int commandsProcessedId;
+  private static final int keyspaceHitsId;
+  private static final int keyspaceMissesId;
+  private static final int totalNetworkBytesReadId;
   private static final int publishRequestsCompletedId;
   private static final int publishRequestsInProgressId;
   private static final int publishRequestTimeId;
   private static final int subscribersId;
   private static final int uniqueChannelSubscriptionsId;
   private static final int uniquePatternSubscriptionsId;
-  private final Statistics stats;
+  private final Statistics generalStats;
+  private final Map<String, Statistics> statistics = new HashMap<>();
   private final StatisticsClock clock;
 
-  public GeodeRedisStats(StatisticsFactory factory, String name, StatisticsClock clock) {
+  public GeodeRedisStats(StatisticsFactory factory, StatisticsClock clock) {
     this.clock = clock;
-    stats = factory == null ? null : factory.createAtomicStatistics(type, name);
+    generalStats = factory == null ? null
+        : factory.createAtomicStatistics(statisticTypes.get(GENERAL_CATEGORY), STATS_BASENAME);

Review comment:
       It looks to me like your textId you pass to createAtomicStatistics is now always the same as your StatisticType name. So instead of calling createAtomicStatistics(StatisticType, String) you should call createAtomicStatistics(StatisticType) which will end up using the name of the type as the textId.
   You would make this change here and on line 78.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();

Review comment:
       instead of using String as the key it seems like Category would be stronger and then you could use EnumMap instead of HashMap

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -218,12 +248,21 @@ private static void fillListWithCompletedCommandDescriptors(StatisticsTypeFactor
     }
   }
 
-  private static void fillCompletedIdMap() {
-    for (RedisCommandType command : RedisCommandType.values()) {
+  private static void fillCompletedIdMap(RedisCommandType.Category category) {
+    for (RedisCommandType command : RedisCommandType.getCommandsForCategory(category)) {
       String name = command.name().toLowerCase();
       String statName = name + "Completed";
+      completedCommandStatIds.put(command,
+          statisticTypes.get(command.category().name()).nameToId(statName));

Review comment:
       or consider my other idea of just passing type in as a parameter

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> completedCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> timeCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
 
-  private static final int currentlyConnectedClients;
+  private static final int currentlyConnectedClientsId;
   private static final int passiveExpirationChecksId;
   private static final int passiveExpirationCheckTimeId;
   private static final int passiveExpirationsId;
   private static final int expirationsId;
   private static final int expirationTimeId;
-  private static final int totalConnectionsReceived;
-  private static final int commandsProcessed;
-  private static final int keyspaceHits;
-  private static final int keyspaceMisses;
-  private static final int totalNetworkBytesRead;
+  private static final int totalConnectionsReceivedId;
+  private static final int commandsProcessedId;
+  private static final int keyspaceHitsId;
+  private static final int keyspaceMissesId;
+  private static final int totalNetworkBytesReadId;
   private static final int publishRequestsCompletedId;
   private static final int publishRequestsInProgressId;
   private static final int publishRequestTimeId;
   private static final int subscribersId;
   private static final int uniqueChannelSubscriptionsId;
   private static final int uniquePatternSubscriptionsId;
-  private final Statistics stats;
+  private final Statistics generalStats;
+  private final Map<String, Statistics> statistics = new HashMap<>();
   private final StatisticsClock clock;
 
-  public GeodeRedisStats(StatisticsFactory factory, String name, StatisticsClock clock) {
+  public GeodeRedisStats(StatisticsFactory factory, StatisticsClock clock) {
     this.clock = clock;
-    stats = factory == null ? null : factory.createAtomicStatistics(type, name);
+    generalStats = factory == null ? null
+        : factory.createAtomicStatistics(statisticTypes.get(GENERAL_CATEGORY), STATS_BASENAME);
+    statistics.put(GENERAL_CATEGORY, generalStats);
+
+    for (RedisCommandType.Category category : RedisCommandType.Category.values()) {
+      String statName = STATS_BASENAME + ":" + category.name();
+      Statistics stats = factory == null ? null
+          : factory.createAtomicStatistics(statisticTypes.get(category.name()), statName);
+      statistics.put(category.name(), stats);
+    }
   }
 
   static {
     StatisticsTypeFactory statisticsTypeFactory = StatisticsTypeFactoryImpl.singleton();
     ArrayList<StatisticDescriptor> descriptorList = new ArrayList<>();
 
-    fillListWithCompletedCommandDescriptors(statisticsTypeFactory, descriptorList);
-    fillListWithTimeCommandDescriptors(statisticsTypeFactory, descriptorList);
     fillListWithCommandDescriptors(statisticsTypeFactory, descriptorList);
+    StatisticDescriptor[] descriptorArray = descriptorList.toArray(new StatisticDescriptor[0]);
 
-    StatisticDescriptor[] descriptorArray =
-        descriptorList.toArray(new StatisticDescriptor[0]);
-
-    type = statisticsTypeFactory
-        .createType("GeodeForRedisStats",
+    StatisticsType generalType = statisticsTypeFactory
+        .createType(STATS_BASENAME,
             "Statistics for a geode-for-redis server",
             descriptorArray);
 
-    fillCompletedIdMap();
-    fillTimeIdMap();
-
-    currentlyConnectedClients = type.nameToId("connectedClients");
-    passiveExpirationChecksId = type.nameToId("passiveExpirationChecks");
-    passiveExpirationCheckTimeId = type.nameToId("passiveExpirationCheckTime");
-    passiveExpirationsId = type.nameToId("passiveExpirations");
-    expirationsId = type.nameToId("expirations");
-    expirationTimeId = type.nameToId("expirationTime");
-    totalConnectionsReceived = type.nameToId("totalConnectionsReceived");
-    commandsProcessed = type.nameToId("commandsProcessed");
-    totalNetworkBytesRead = type.nameToId("totalNetworkBytesRead");
-    keyspaceHits = type.nameToId("keyspaceHits");
-    keyspaceMisses = type.nameToId("keyspaceMisses");
-    publishRequestsCompletedId = type.nameToId("publishRequestsCompleted");
-    publishRequestsInProgressId = type.nameToId("publishRequestsInProgress");
-    publishRequestTimeId = type.nameToId("publishRequestTime");
-    subscribersId = type.nameToId("subscribers");
-    uniqueChannelSubscriptionsId = type.nameToId("uniqueChannelSubscriptions");
-    uniquePatternSubscriptionsId = type.nameToId("uniquePatternSubscriptions");
+    currentlyConnectedClientsId = generalType.nameToId("connectedClients");
+    passiveExpirationChecksId = generalType.nameToId("passiveExpirationChecks");
+    passiveExpirationCheckTimeId = generalType.nameToId("passiveExpirationCheckTime");
+    passiveExpirationsId = generalType.nameToId("passiveExpirations");
+    expirationsId = generalType.nameToId("expirations");
+    expirationTimeId = generalType.nameToId("expirationTime");
+    totalConnectionsReceivedId = generalType.nameToId("totalConnectionsReceived");
+    commandsProcessedId = generalType.nameToId("commandsProcessed");
+    totalNetworkBytesReadId = generalType.nameToId("totalNetworkBytesRead");
+    keyspaceHitsId = generalType.nameToId("keyspaceHits");
+    keyspaceMissesId = generalType.nameToId("keyspaceMisses");
+    publishRequestsCompletedId = generalType.nameToId("publishRequestsCompleted");
+    publishRequestsInProgressId = generalType.nameToId("publishRequestsInProgress");
+    publishRequestTimeId = generalType.nameToId("publishRequestTime");
+    subscribersId = generalType.nameToId("subscribers");
+    uniqueChannelSubscriptionsId = generalType.nameToId("uniqueChannelSubscriptions");
+    uniquePatternSubscriptionsId = generalType.nameToId("uniquePatternSubscriptions");
+
+    statisticTypes.put(GENERAL_CATEGORY, generalType);
+
+    for (RedisCommandType.Category category : RedisCommandType.Category.values()) {
+      ArrayList<StatisticDescriptor> descriptors = new ArrayList<>();
+      fillListWithCompletedCommandDescriptors(statisticsTypeFactory, category, descriptors);
+      fillListWithTimeCommandDescriptors(statisticsTypeFactory, category, descriptors);
+
+      StatisticDescriptor[] descriptorsArray = descriptors.toArray(new StatisticDescriptor[0]);
+      StatisticsType type = statisticsTypeFactory
+          .createType(STATS_BASENAME + ":" + category.name(),
+              category.name() + " statistics for a geode-for-redis server",
+              descriptorsArray);
+      statisticTypes.put(category.name(), type);
+
+      fillCompletedIdMap(category);

Review comment:
       consider just passing "type" in to these two fill methods instead of them turning around and looking it up.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> completedCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> timeCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
 
-  private static final int currentlyConnectedClients;
+  private static final int currentlyConnectedClientsId;
   private static final int passiveExpirationChecksId;
   private static final int passiveExpirationCheckTimeId;
   private static final int passiveExpirationsId;
   private static final int expirationsId;
   private static final int expirationTimeId;
-  private static final int totalConnectionsReceived;
-  private static final int commandsProcessed;
-  private static final int keyspaceHits;
-  private static final int keyspaceMisses;
-  private static final int totalNetworkBytesRead;
+  private static final int totalConnectionsReceivedId;
+  private static final int commandsProcessedId;
+  private static final int keyspaceHitsId;
+  private static final int keyspaceMissesId;
+  private static final int totalNetworkBytesReadId;
   private static final int publishRequestsCompletedId;
   private static final int publishRequestsInProgressId;
   private static final int publishRequestTimeId;
   private static final int subscribersId;
   private static final int uniqueChannelSubscriptionsId;
   private static final int uniquePatternSubscriptionsId;
-  private final Statistics stats;
+  private final Statistics generalStats;
+  private final Map<String, Statistics> statistics = new HashMap<>();

Review comment:
       instead of using String as the key, Category would be stronger and then you could use EnumMap instead of HashMap

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> completedCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> timeCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
 
-  private static final int currentlyConnectedClients;
+  private static final int currentlyConnectedClientsId;
   private static final int passiveExpirationChecksId;
   private static final int passiveExpirationCheckTimeId;
   private static final int passiveExpirationsId;
   private static final int expirationsId;
   private static final int expirationTimeId;
-  private static final int totalConnectionsReceived;
-  private static final int commandsProcessed;
-  private static final int keyspaceHits;
-  private static final int keyspaceMisses;
-  private static final int totalNetworkBytesRead;
+  private static final int totalConnectionsReceivedId;
+  private static final int commandsProcessedId;
+  private static final int keyspaceHitsId;
+  private static final int keyspaceMissesId;
+  private static final int totalNetworkBytesReadId;
   private static final int publishRequestsCompletedId;
   private static final int publishRequestsInProgressId;
   private static final int publishRequestTimeId;
   private static final int subscribersId;
   private static final int uniqueChannelSubscriptionsId;
   private static final int uniquePatternSubscriptionsId;
-  private final Statistics stats;
+  private final Statistics generalStats;
+  private final Map<String, Statistics> statistics = new HashMap<>();
   private final StatisticsClock clock;
 
-  public GeodeRedisStats(StatisticsFactory factory, String name, StatisticsClock clock) {
+  public GeodeRedisStats(StatisticsFactory factory, StatisticsClock clock) {
     this.clock = clock;
-    stats = factory == null ? null : factory.createAtomicStatistics(type, name);
+    generalStats = factory == null ? null

Review comment:
       factory should not be null. If it is, it must be because of a test that did not mock getStatisticsManager() on InternalDistributedSystem.
   I think you should clean this method up to expect factory to not be null and fix any test that has incomplete mocking

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -218,12 +248,21 @@ private static void fillListWithCompletedCommandDescriptors(StatisticsTypeFactor
     }
   }
 
-  private static void fillCompletedIdMap() {
-    for (RedisCommandType command : RedisCommandType.values()) {
+  private static void fillCompletedIdMap(RedisCommandType.Category category) {
+    for (RedisCommandType command : RedisCommandType.getCommandsForCategory(category)) {
       String name = command.name().toLowerCase();
       String statName = name + "Completed";
+      completedCommandStatIds.put(command,
+          statisticTypes.get(command.category().name()).nameToId(statName));

Review comment:
       Could the lookup of the type be moved outside the for loop and "category" used instead of "command.category"?

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> completedCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> timeCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
 
-  private static final int currentlyConnectedClients;
+  private static final int currentlyConnectedClientsId;
   private static final int passiveExpirationChecksId;
   private static final int passiveExpirationCheckTimeId;
   private static final int passiveExpirationsId;
   private static final int expirationsId;
   private static final int expirationTimeId;
-  private static final int totalConnectionsReceived;
-  private static final int commandsProcessed;
-  private static final int keyspaceHits;
-  private static final int keyspaceMisses;
-  private static final int totalNetworkBytesRead;
+  private static final int totalConnectionsReceivedId;
+  private static final int commandsProcessedId;
+  private static final int keyspaceHitsId;
+  private static final int keyspaceMissesId;
+  private static final int totalNetworkBytesReadId;
   private static final int publishRequestsCompletedId;
   private static final int publishRequestsInProgressId;
   private static final int publishRequestTimeId;
   private static final int subscribersId;
   private static final int uniqueChannelSubscriptionsId;
   private static final int uniquePatternSubscriptionsId;
-  private final Statistics stats;
+  private final Statistics generalStats;
+  private final Map<String, Statistics> statistics = new HashMap<>();
   private final StatisticsClock clock;
 
-  public GeodeRedisStats(StatisticsFactory factory, String name, StatisticsClock clock) {
+  public GeodeRedisStats(StatisticsFactory factory, StatisticsClock clock) {
     this.clock = clock;
-    stats = factory == null ? null : factory.createAtomicStatistics(type, name);
+    generalStats = factory == null ? null
+        : factory.createAtomicStatistics(statisticTypes.get(GENERAL_CATEGORY), STATS_BASENAME);
+    statistics.put(GENERAL_CATEGORY, generalStats);
+
+    for (RedisCommandType.Category category : RedisCommandType.Category.values()) {
+      String statName = STATS_BASENAME + ":" + category.name();
+      Statistics stats = factory == null ? null
+          : factory.createAtomicStatistics(statisticTypes.get(category.name()), statName);
+      statistics.put(category.name(), stats);
+    }
   }
 
   static {
     StatisticsTypeFactory statisticsTypeFactory = StatisticsTypeFactoryImpl.singleton();
     ArrayList<StatisticDescriptor> descriptorList = new ArrayList<>();
 
-    fillListWithCompletedCommandDescriptors(statisticsTypeFactory, descriptorList);
-    fillListWithTimeCommandDescriptors(statisticsTypeFactory, descriptorList);
     fillListWithCommandDescriptors(statisticsTypeFactory, descriptorList);

Review comment:
       refactor this method to just return a StatisticDescriptor[] that it will create. Then the ugly ArrayList and toArray code can be in fillListWithCommandDescriptors. And give the method a better name like createGeneralStatisticsDescriptors

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/GeodeRedisStats.java
##########
@@ -30,170 +32,197 @@
 import org.apache.geode.redis.internal.commands.RedisCommandType;
 
 public class GeodeRedisStats {
+
+  public static final String STATS_BASENAME = "GeodeForRedisStats";
+  private static final String GENERAL_CATEGORY = "General";
+
   @Immutable
-  private static final StatisticsType type;
+  private static final Map<String, StatisticsType> statisticTypes = new HashMap<>();
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> completedCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
   @Immutable
   private static final EnumMap<RedisCommandType, Integer> timeCommandStatIds =
       new EnumMap<>(RedisCommandType.class);
 
-  private static final int currentlyConnectedClients;
+  private static final int currentlyConnectedClientsId;
   private static final int passiveExpirationChecksId;
   private static final int passiveExpirationCheckTimeId;
   private static final int passiveExpirationsId;
   private static final int expirationsId;
   private static final int expirationTimeId;
-  private static final int totalConnectionsReceived;
-  private static final int commandsProcessed;
-  private static final int keyspaceHits;
-  private static final int keyspaceMisses;
-  private static final int totalNetworkBytesRead;
+  private static final int totalConnectionsReceivedId;
+  private static final int commandsProcessedId;
+  private static final int keyspaceHitsId;
+  private static final int keyspaceMissesId;
+  private static final int totalNetworkBytesReadId;
   private static final int publishRequestsCompletedId;
   private static final int publishRequestsInProgressId;
   private static final int publishRequestTimeId;
   private static final int subscribersId;
   private static final int uniqueChannelSubscriptionsId;
   private static final int uniquePatternSubscriptionsId;
-  private final Statistics stats;
+  private final Statistics generalStats;
+  private final Map<String, Statistics> statistics = new HashMap<>();
   private final StatisticsClock clock;
 
-  public GeodeRedisStats(StatisticsFactory factory, String name, StatisticsClock clock) {
+  public GeodeRedisStats(StatisticsFactory factory, StatisticsClock clock) {
     this.clock = clock;
-    stats = factory == null ? null : factory.createAtomicStatistics(type, name);
+    generalStats = factory == null ? null
+        : factory.createAtomicStatistics(statisticTypes.get(GENERAL_CATEGORY), STATS_BASENAME);
+    statistics.put(GENERAL_CATEGORY, generalStats);
+
+    for (RedisCommandType.Category category : RedisCommandType.Category.values()) {
+      String statName = STATS_BASENAME + ":" + category.name();
+      Statistics stats = factory == null ? null
+          : factory.createAtomicStatistics(statisticTypes.get(category.name()), statName);
+      statistics.put(category.name(), stats);
+    }
   }
 
   static {
     StatisticsTypeFactory statisticsTypeFactory = StatisticsTypeFactoryImpl.singleton();
     ArrayList<StatisticDescriptor> descriptorList = new ArrayList<>();
 
-    fillListWithCompletedCommandDescriptors(statisticsTypeFactory, descriptorList);
-    fillListWithTimeCommandDescriptors(statisticsTypeFactory, descriptorList);
     fillListWithCommandDescriptors(statisticsTypeFactory, descriptorList);
+    StatisticDescriptor[] descriptorArray = descriptorList.toArray(new StatisticDescriptor[0]);
 
-    StatisticDescriptor[] descriptorArray =
-        descriptorList.toArray(new StatisticDescriptor[0]);
-
-    type = statisticsTypeFactory
-        .createType("GeodeForRedisStats",
+    StatisticsType generalType = statisticsTypeFactory
+        .createType(STATS_BASENAME,
             "Statistics for a geode-for-redis server",
             descriptorArray);
 
-    fillCompletedIdMap();
-    fillTimeIdMap();
-
-    currentlyConnectedClients = type.nameToId("connectedClients");
-    passiveExpirationChecksId = type.nameToId("passiveExpirationChecks");
-    passiveExpirationCheckTimeId = type.nameToId("passiveExpirationCheckTime");
-    passiveExpirationsId = type.nameToId("passiveExpirations");
-    expirationsId = type.nameToId("expirations");
-    expirationTimeId = type.nameToId("expirationTime");
-    totalConnectionsReceived = type.nameToId("totalConnectionsReceived");
-    commandsProcessed = type.nameToId("commandsProcessed");
-    totalNetworkBytesRead = type.nameToId("totalNetworkBytesRead");
-    keyspaceHits = type.nameToId("keyspaceHits");
-    keyspaceMisses = type.nameToId("keyspaceMisses");
-    publishRequestsCompletedId = type.nameToId("publishRequestsCompleted");
-    publishRequestsInProgressId = type.nameToId("publishRequestsInProgress");
-    publishRequestTimeId = type.nameToId("publishRequestTime");
-    subscribersId = type.nameToId("subscribers");
-    uniqueChannelSubscriptionsId = type.nameToId("uniqueChannelSubscriptions");
-    uniquePatternSubscriptionsId = type.nameToId("uniquePatternSubscriptions");
+    currentlyConnectedClientsId = generalType.nameToId("connectedClients");
+    passiveExpirationChecksId = generalType.nameToId("passiveExpirationChecks");
+    passiveExpirationCheckTimeId = generalType.nameToId("passiveExpirationCheckTime");
+    passiveExpirationsId = generalType.nameToId("passiveExpirations");
+    expirationsId = generalType.nameToId("expirations");
+    expirationTimeId = generalType.nameToId("expirationTime");
+    totalConnectionsReceivedId = generalType.nameToId("totalConnectionsReceived");
+    commandsProcessedId = generalType.nameToId("commandsProcessed");
+    totalNetworkBytesReadId = generalType.nameToId("totalNetworkBytesRead");
+    keyspaceHitsId = generalType.nameToId("keyspaceHits");
+    keyspaceMissesId = generalType.nameToId("keyspaceMisses");
+    publishRequestsCompletedId = generalType.nameToId("publishRequestsCompleted");
+    publishRequestsInProgressId = generalType.nameToId("publishRequestsInProgress");
+    publishRequestTimeId = generalType.nameToId("publishRequestTime");
+    subscribersId = generalType.nameToId("subscribers");
+    uniqueChannelSubscriptionsId = generalType.nameToId("uniqueChannelSubscriptions");
+    uniquePatternSubscriptionsId = generalType.nameToId("uniquePatternSubscriptions");
+
+    statisticTypes.put(GENERAL_CATEGORY, generalType);
+
+    for (RedisCommandType.Category category : RedisCommandType.Category.values()) {
+      ArrayList<StatisticDescriptor> descriptors = new ArrayList<>();
+      fillListWithCompletedCommandDescriptors(statisticsTypeFactory, category, descriptors);

Review comment:
       consider refactoring fillListWithCompletedCommandDescriptors and fillListWithTimeCommandDescriptors into "StatisticsDescriptor[] createCategoryStatisticsDescriptors(factory, category)". Then you can get rid of the ugly ArrayList and toArray at this level and the code the two fill methods have in common can no longer be duplicated. Foreach command you can call "StatisticDescriptor createTimeDescriptor(factory, name)" and "createCompletedDescriptor(factory, name)". This seems a little cleaner to me. 




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org