[jira] [Commented] (GEODE-8663) update Redis Info command To include additional statistics

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

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

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

jdeppe-pivotal commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518878564



##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/AbstractExistsIntegrationTest.java
##########
@@ -61,7 +61,8 @@ public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
 
   @Test
   public void shouldReturnZero_givenKeyDoesNotExist() {
-    assertThat(jedis.exists(toArray("doesNotExist"))).isEqualTo(0L);
+    assertThat(

Review comment:
       Please don't add formatting changes that seem somewhat arbitrary to the overall PR. 

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKeyCommandsFunctionExecutor.java
##########
@@ -33,12 +33,34 @@ public boolean del(ByteArrayWrapper key) {
 
   @Override
   public boolean exists(ByteArrayWrapper key) {
-    return stripedExecute(key, () -> getRedisData(key).exists());
+    boolean keyExists =

Review comment:
       Please keep this line formatted as it was.

##########
File path: geode-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -47,6 +53,14 @@ protected void before() {
     server.setAllowUnsupportedCommands(true);
   }
 
+  public long getStartTime() {

Review comment:
       I don't think this is the right place to add these methods. Can the test that utilizes them not simply maintain local values for these (and initialize them in a `@BeforeClass` block?

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKeyCommandsFunctionExecutor.java
##########
@@ -33,12 +33,34 @@ public boolean del(ByteArrayWrapper key) {
 
   @Override
   public boolean exists(ByteArrayWrapper key) {
-    return stripedExecute(key, () -> getRedisData(key).exists());
+    boolean keyExists =
+        stripedExecute(
+            key,
+            () -> getRedisData(key).exists());
+
+    if (keyExists) {
+      helper.getRedisStats().incKeyspaceHits();
+    } else {
+      helper.getRedisStats().incKeyspaceMisses();
+    }
+
+    return keyExists;
   }
 
   @Override
   public long pttl(ByteArrayWrapper key) {
-    return stripedExecute(key, () -> getRedisData(key).pttl(getRegion(), key));
+    long result =

Review comment:
       Please keep this line formatted as it was.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/GetBitExecutor.java
##########
@@ -40,7 +40,8 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
       return RedisResponse.error(ERROR_NOT_INT);
     }
 
-    int result = getRedisStringCommands(context).getbit(key, offset);
+    int result = getRedisStringCommands(context)
+        .getbit(key, offset);

Review comment:
       Please don't add formatting changes that seem somewhat arbitrary to the overall PR. 

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -82,6 +104,28 @@
     expirationTimeId = type.nameToId("expirationTime");
   }
 
+  public RedisStats(StatisticsFactory factory, StatisticsClock clock) {
+    this(factory, "redisStats", clock);
+  }
+
+  public RedisStats(StatisticsFactory factory, String textId, StatisticsClock clock) {
+    stats = factory == null ? null : factory.createAtomicStatistics(type, textId);
+    this.clock = clock;
+    this.START_TIME_IN_NANOS = this.clock.getTime();
+    perSecondExecutor = startPerSecondUpdater();
+  }
+
+  public void clearAllStats() {

Review comment:
       For consistency, I think it would be good to reset the other (Geode-specific) stats here too (not just `clientId`).

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -69,29 +75,97 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
     return result;
   }
 
+  private String getStatsSection(ExecutionHandlerContext context) {
+    final RedisStats redisStats = context.getRedisStats();
+    String instantaneous_input_kbps =
+        new DecimalFormat("0.00")

Review comment:
       It would be better to create a private instance variable for this `DecimalForamt` and then re-use that here when calling `.format`. Don't make it `static` since `DecimalFormat` is not thread-safe.




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


> update Redis Info command To include additional statistics
> ----------------------------------------------------------
>
>                 Key: GEODE-8663
>                 URL: https://issues.apache.org/jira/browse/GEODE-8663
>             Project: Geode
>          Issue Type: Improvement
>          Components: redis
>            Reporter: John Hutchison
>            Priority: Major
>              Labels: pull-request-available
>




--
This message was sent by Atlassian Jira
(v8.3.4#803005)