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 2020/10/27 21:35:39 UTC
[GitHub] [geode] jhutchison opened a new pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
jhutchison opened a new pull request #5678:
URL: https://github.com/apache/geode/pull/5678
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513757239
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
I don't think we are counting this statistic properly. According to the Redis `INFO` docs (and also testing our version vs. Redis), it should be `the number of keys with an expiration` since we are using it in the `keyspace` section:
```
The keyspace section provides statistics on the main dictionary of each database. The statistics are the number of keys, and the number of keys with an expiration.
For each database, the following line is added:
dbXXX: keys=XXX,expires=XXX
```
This means that we should add any key that has an expiration set and remove it when it no longer has an expiration (because it was persisted, expired on its own, was deleted, etc.)
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514480604
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -171,13 +180,92 @@ public void endCommand(RedisCommandType command, long start) {
}
public void addClient() {
+ connectionsReceived.incrementAndGet();
+ connectedClients.incrementAndGet();
stats.incLong(clientId, 1);
}
public void removeClient() {
+ connectedClients.decrementAndGet();
stats.incLong(clientId, -1);
}
+ private final AtomicLong commandsProcessed = new AtomicLong();
+ private final AtomicLong opsPerSecond = new AtomicLong();
+ private final AtomicLong networkBytesRead = new AtomicLong();
+ private volatile double networkKilobytesReadPerSecond;
+ private final AtomicLong connectionsReceived = new AtomicLong();
+ private final AtomicLong connectedClients = new AtomicLong();
+ private final AtomicLong expirations = new AtomicLong();
+ private final AtomicLong keyspaceHits = new AtomicLong();
+ private final AtomicLong keyspaceMisses = new AtomicLong();
Review comment:
will add
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518941702
##########
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:
agree, thanks.
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514479384
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor.java
##########
@@ -11,7 +11,6 @@
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
- *
Review comment:
good catch- not sure if that was a merge error? weird
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513829391
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -58,6 +61,9 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
case "persistence":
result = getPersistenceSection();
break;
+ case "replication":
+ result = getReplicationSection();
+ break;
Review comment:
We also need to add the following cases: `clients`, `memory`, `stats`, and `keyspace`
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518935876
##########
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:
done
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514484847
##########
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(
+ jedis.exists(toArray("doesNotExist"))).isEqualTo(0L);
Review comment:
will submit separate PR with format changes
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513757239
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
I don't think we are counting this statistic properly. According to the Redis `INFO` docs (and also testing our version vs. Redis), it should be `the number of keys with an expiration` since we are using it in the `keyspace` section:
```
The keyspace section provides statistics on the main dictionary of each database. The statistics are the number of keys, and the number of keys with an expiration.
For each database, the following line is added:
dbXXX: keys=XXX,expires=XXX
```
This means that we should increment the count when any key has an expiration set and decrement it when that key no longer has an expiration (because it was persisted, expired on its own, was deleted, etc.). Right now, we only increment the count when a key expires and we never decrement 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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514491157
##########
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:
sure
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514487489
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
story added to backlog for this
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514492081
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -72,26 +78,91 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
private String getServerSection(ExecutionHandlerContext context) {
final String CURRENT_REDIS_VERSION = "5.0.6";
final int TCP_PORT = context.getServerPort();
+ final RedisStats redisStats = context.getRedisStats();
final String SERVER_STRING =
"# Server\r\n" +
"redis_version:" + CURRENT_REDIS_VERSION + "\r\n" +
"redis_mode:standalone\r\n" +
- "tcp_port:" + TCP_PORT + "\r\n";
+ "tcp_port:" + TCP_PORT + "\r\n" +
+ "uptime_in_seconds:" + redisStats.getUptimeInSeconds() + "\r\n" +
+ "uptime_in_days:" + redisStats.getUptimeInDays() + "\r\n";
return SERVER_STRING;
}
+ private String getClientsSection(ExecutionHandlerContext context) {
+ final RedisStats redisStats = context.getRedisStats();
+ final String CLIENTS_STRING =
+ "# Clients\r\n" +
+ "connected_clients:" + redisStats.getConnectedClients() + "\r\n" +
+ "blocked_clients:0\r\n";
+ return CLIENTS_STRING;
+ }
+
+ private String getMemorySection(ExecutionHandlerContext context) {
+ PartitionedRegion pr = (PartitionedRegion) context.getRegionProvider().getDataRegion();
+ long usedMemory = pr.getDataStore().currentAllocatedMemory();
+ final String MEMORY_STRING =
+ "# Memory\r\n" +
+ "used_memory:" + usedMemory + "\r\n" +
+ "mem_fragmentation_ratio:0\r\n";
+ return MEMORY_STRING;
+ }
+
+ private String getStatsSection(ExecutionHandlerContext context) {
+ final RedisStats redisStats = context.getRedisStats();
+ String instantaneous_input_kbps =
+ new DecimalFormat("0.00")
+ .format(redisStats.getNetworkKilobytesReadPerSecond());
+ final String STATS_STRING =
+ "# Stats\r\n" +
+ "total_commands_processed:" + redisStats.getCommandsProcessed() + "\r\n" +
+ "instantaneous_ops_per_sec:" + redisStats.getOpsPerSecond() + "\r\n" +
+ "total_net_input_bytes:" + redisStats.getNetworkBytesRead() + "\r\n" +
+ "instantaneous_input_kbps:" + instantaneous_input_kbps + "\r\n" +
+ "total_connections_received:" + redisStats.getConnectionsReceived() + "\r\n" +
+ "keyspace_hits:" + redisStats.getKeyspaceHits() + "\r\n" +
+ "keyspace_misses:" + redisStats.getKeyspaceMisses() + "\r\n" +
+ "evicted_keys:0\r\n" +
+ "rejected_connections:0\r\n";
+ return STATS_STRING;
+ }
+
+ private String getKeyspaceSection(ExecutionHandlerContext context) {
+ final RedisStats redisStats = context.getRedisStats();
+ final String KEYSPACE_STRING =
+ "# Keyspace\r\n" +
+ "db0:keys=" + context.getRegionProvider().getDataRegion().size() +
+ ",expires=" + redisStats.getExpirations() +
+ ",avg_ttl=0\r\n";
Review comment:
story made for this
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514484086
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513731184
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
Review comment:
I mentioned in another comment that we should test all the statistics, not just clients and keyspace hits and misses. We should also add tests/update existing tests in `AbstractInfoIntegrationTest`
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514478970
##########
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();
+ }
Review comment:
will address in future stories that are coming to investigate overall functionality of this piece
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518940357
##########
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:
changed back
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518935876
##########
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:
meant to pull these out for another commit- sorry
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514489310
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/SelectExecutor.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ *
+ */
+package org.apache.geode.redis.internal.executor.connection;
+
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class SelectExecutor extends AbstractExecutor {
Review comment:
thanks for catching this- will remove
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514490857
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -171,13 +180,92 @@ public void endCommand(RedisCommandType command, long start) {
}
public void addClient() {
+ connectionsReceived.incrementAndGet();
+ connectedClients.incrementAndGet();
stats.incLong(clientId, 1);
Review comment:
will likely be addressed in future work on functionality here
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513564532
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
Review comment:
We should use the awaitility timeout here: `Math.toIntExact(GeodeAwaitility.getTimeout().toMillis())`
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
+ jedis.flushAll();
+ jedis.set(EXISTING_STRING_KEY, "A_Value");
+ jedis.hset(EXISTING_HASH_KEY, "Field1", "Value1");
+ jedis.sadd(EXISTING_SET_KEY_1, "m1", "m2", "m3");
+ jedis.sadd(EXISTING_SET_KEY_2, "m4", "m5", "m6");
+ initialKeyspaceHits = server.getServer().getStats().getKeyspaceHits();
+ initialKeyspaceMisses = server.getServer().getStats().getKeyspaceMisses();
+ }
+
@Test
- public void clientsStat_withConnectAndClose_isCorrect() throws InterruptedException {
+ public void clientsStat_withConnectAndClose_isCorrect() {
long initialClients = server.getServer().getStats().getClients();
Jedis jedis = new Jedis("localhost", server.getPort(), 10000000);
Review comment:
I don't think this Jedis instance is necessary since we've already created one in the test setup.
##########
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(
+ jedis.exists(toArray("doesNotExist"))).isEqualTo(0L);
Review comment:
Since nothing else was changed in this file, and it's just a formatting change, maybe this could be reverted and we could allow spA to handle the formatting.
##########
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:
Seems like we could leave this formatting change to spA.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/SlowlogExecutor.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.server;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+// TODO: only exists for Redis monitoring software, maybe make functional someday?
+public class SlowlogExecutor extends AbstractExecutor {
Review comment:
Do we actually need this executor for monitoring software? It doesn't seem like we use it anywhere. The command itself is still listed as `UNSUPPORTED` in `RedisCommandType`, so it would return an error.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/CommandHelper.java
##########
@@ -84,7 +84,13 @@ RedisData getRedisData(ByteArrayWrapper key, RedisData notFoundValue) {
}
RedisSet getRedisSet(ByteArrayWrapper key) {
- return checkSetType(getRedisData(key, NULL_REDIS_SET));
+ RedisData redisData = getRedisData(key, NULL_REDIS_SET);
+ if (redisData == NULL_REDIS_SET) {
+ redisStats.incKeyspaceMisses();
+ } else {
+ redisStats.incKeyspaceHits();
Review comment:
If we decide to increment the hits/misses this way, we could consolidate all of these and just have the if/else in the `getRedisData` method.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -72,26 +78,91 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
private String getServerSection(ExecutionHandlerContext context) {
final String CURRENT_REDIS_VERSION = "5.0.6";
final int TCP_PORT = context.getServerPort();
+ final RedisStats redisStats = context.getRedisStats();
final String SERVER_STRING =
"# Server\r\n" +
"redis_version:" + CURRENT_REDIS_VERSION + "\r\n" +
"redis_mode:standalone\r\n" +
- "tcp_port:" + TCP_PORT + "\r\n";
+ "tcp_port:" + TCP_PORT + "\r\n" +
+ "uptime_in_seconds:" + redisStats.getUptimeInSeconds() + "\r\n" +
+ "uptime_in_days:" + redisStats.getUptimeInDays() + "\r\n";
return SERVER_STRING;
}
+ private String getClientsSection(ExecutionHandlerContext context) {
+ final RedisStats redisStats = context.getRedisStats();
+ final String CLIENTS_STRING =
+ "# Clients\r\n" +
+ "connected_clients:" + redisStats.getConnectedClients() + "\r\n" +
+ "blocked_clients:0\r\n";
+ return CLIENTS_STRING;
+ }
+
+ private String getMemorySection(ExecutionHandlerContext context) {
+ PartitionedRegion pr = (PartitionedRegion) context.getRegionProvider().getDataRegion();
+ long usedMemory = pr.getDataStore().currentAllocatedMemory();
+ final String MEMORY_STRING =
+ "# Memory\r\n" +
+ "used_memory:" + usedMemory + "\r\n" +
+ "mem_fragmentation_ratio:0\r\n";
+ return MEMORY_STRING;
+ }
+
+ private String getStatsSection(ExecutionHandlerContext context) {
+ final RedisStats redisStats = context.getRedisStats();
+ String instantaneous_input_kbps =
+ new DecimalFormat("0.00")
+ .format(redisStats.getNetworkKilobytesReadPerSecond());
+ final String STATS_STRING =
+ "# Stats\r\n" +
+ "total_commands_processed:" + redisStats.getCommandsProcessed() + "\r\n" +
+ "instantaneous_ops_per_sec:" + redisStats.getOpsPerSecond() + "\r\n" +
+ "total_net_input_bytes:" + redisStats.getNetworkBytesRead() + "\r\n" +
+ "instantaneous_input_kbps:" + instantaneous_input_kbps + "\r\n" +
+ "total_connections_received:" + redisStats.getConnectionsReceived() + "\r\n" +
+ "keyspace_hits:" + redisStats.getKeyspaceHits() + "\r\n" +
+ "keyspace_misses:" + redisStats.getKeyspaceMisses() + "\r\n" +
+ "evicted_keys:0\r\n" +
+ "rejected_connections:0\r\n";
+ return STATS_STRING;
+ }
+
+ private String getKeyspaceSection(ExecutionHandlerContext context) {
+ final RedisStats redisStats = context.getRedisStats();
+ final String KEYSPACE_STRING =
+ "# Keyspace\r\n" +
+ "db0:keys=" + context.getRegionProvider().getDataRegion().size() +
+ ",expires=" + redisStats.getExpirations() +
+ ",avg_ttl=0\r\n";
Review comment:
In Redis, if there are no keys in the database, info just displays `# Keyspace` with nothing after it. I like the idea of displaying 0 for those stats rather than nothing at all, but I'm not sure if there is any reason we need to emulate Redis's behavior?
##########
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();
+ }
Review comment:
Similar to the `CommandHelper` class, if we decide to increment the hits/misses this way, we could consolidate all of these and just have the if/else in the `getRedisData` method.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor.java
##########
@@ -11,7 +11,6 @@
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
- *
Review comment:
It seems strange that this was deleted.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -169,7 +169,8 @@ public void initChannel(SocketChannel socketChannel) {
}
ChannelPipeline pipeline = socketChannel.pipeline();
addSSLIfEnabled(socketChannel, pipeline);
- pipeline.addLast(ByteToCommandDecoder.class.getSimpleName(), new ByteToCommandDecoder());
+ pipeline.addLast(ByteToCommandDecoder.class.getSimpleName(),
+ new ByteToCommandDecoder(redisStats));
Review comment:
Seems like we could let spA handle this formatting change, especially since nothing else was changed in this file.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -171,13 +180,92 @@ public void endCommand(RedisCommandType command, long start) {
}
public void addClient() {
+ connectionsReceived.incrementAndGet();
+ connectedClients.incrementAndGet();
stats.incLong(clientId, 1);
Review comment:
The `clientID` statistic is actually the same as `connectedClients`. We could make the `getClients()` method public and just use that.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/SelectExecutor.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ *
+ */
+package org.apache.geode.redis.internal.executor.connection;
+
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class SelectExecutor extends AbstractExecutor {
Review comment:
Not sure if we want to add the `SelectExecutor` in this PR, since there's another PR that adds it. If we do add it in this PR, we need to update `SupportedCommandsJUnitTest` to remove `SELECT` from the list of unimplemented commands and add it to the list of unsupported commands.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -171,13 +180,92 @@ public void endCommand(RedisCommandType command, long start) {
}
public void addClient() {
+ connectionsReceived.incrementAndGet();
+ connectedClients.incrementAndGet();
stats.incLong(clientId, 1);
}
public void removeClient() {
+ connectedClients.decrementAndGet();
stats.incLong(clientId, -1);
}
+ private final AtomicLong commandsProcessed = new AtomicLong();
+ private final AtomicLong opsPerSecond = new AtomicLong();
+ private final AtomicLong networkBytesRead = new AtomicLong();
+ private volatile double networkKilobytesReadPerSecond;
+ private final AtomicLong connectionsReceived = new AtomicLong();
+ private final AtomicLong connectedClients = new AtomicLong();
+ private final AtomicLong expirations = new AtomicLong();
+ private final AtomicLong keyspaceHits = new AtomicLong();
+ private final AtomicLong keyspaceMisses = new AtomicLong();
Review comment:
We have a lot of integration tests for `keyspaceHits` and `keyspaceMisses`, but we don't have any for the other statistics. It would be good to test those as well.
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
Review comment:
I mentioned below testing all the statistics, not just clients and keyspace hits and misses. We should also add additional tests/update existing tests in `AbstractInfoIntegrationTest`
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513757239
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
I don't think we are counting this statistic properly. According to the Redis `INFO` docs (and also testing our version vs. Redis), it should be `the number of keys with an expiration` since we are using it in the `keyspace` section:
```
The keyspace section provides statistics on the main dictionary of each database. The statistics are the number of keys, and the number of keys with an expiration.
For each database, the following line is added:
dbXXX: keys=XXX,expires=XXX
```
This means that we should add a key when it is set to expire and remove it when it no longer has an expiration (because it was persisted, expired on its own, was deleted, etc.)
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513682294
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/SlowlogExecutor.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.server;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+// TODO: only exists for Redis monitoring software, maybe make functional someday?
+public class SlowlogExecutor extends AbstractExecutor {
Review comment:
Do we actually need this executor for monitoring software? It doesn't seem like we use it anywhere. The command itself is still listed as `UNIMPLEMENTED` in `RedisCommandType`, so it would return an error.
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r519924460
##########
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:
Not necessary to do this in this PR unless you're making other changes as well, but it might be a good idea to make this method `protected` instead of public and mark it as `@VisibleForTesting`
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 merged pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 merged pull request #5678:
URL: https://github.com/apache/geode/pull/5678
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518935500
##########
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:
done
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513724885
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -171,13 +180,92 @@ public void endCommand(RedisCommandType command, long start) {
}
public void addClient() {
+ connectionsReceived.incrementAndGet();
+ connectedClients.incrementAndGet();
stats.incLong(clientId, 1);
Review comment:
The `clientID` statistic is actually the same as `connectedClients`. We could make the `getClients()` method public and just use that (unless there's a reason we shouldn't make it public)
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513757239
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
I don't think we are counting expirations properly. According to the Redis `INFO` docs (and also testing our version vs. Redis), it should be `the number of keys with an expiration` since we are using it in the `keyspace` section:
```
The keyspace section provides statistics on the main dictionary of each database. The statistics are the number of keys, and the number of keys with an expiration.
For each database, the following line is added:
dbXXX: keys=XXX,expires=XXX
```
This means that we should increment the count when any key has an expiration set and decrement it when that key no longer has an expiration (because it was persisted, expired on its own, was deleted, etc.). Right now, we only increment the count when a key expires and we never decrement 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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518935984
##########
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:
same as above
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514498015
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
+ jedis.flushAll();
+ jedis.set(EXISTING_STRING_KEY, "A_Value");
+ jedis.hset(EXISTING_HASH_KEY, "Field1", "Value1");
+ jedis.sadd(EXISTING_SET_KEY_1, "m1", "m2", "m3");
+ jedis.sadd(EXISTING_SET_KEY_2, "m4", "m5", "m6");
+ initialKeyspaceHits = server.getServer().getStats().getKeyspaceHits();
+ initialKeyspaceMisses = server.getServer().getStats().getKeyspaceMisses();
+ }
+
@Test
- public void clientsStat_withConnectAndClose_isCorrect() throws InterruptedException {
+ public void clientsStat_withConnectAndClose_isCorrect() {
long initialClients = server.getServer().getStats().getClients();
Jedis jedis = new Jedis("localhost", server.getPort(), 10000000);
jedis.ping();
assertThat(server.getServer().getStats().getClients()).isEqualTo(initialClients + 1);
jedis.close();
- GeodeAwaitility.await().untilAsserted(
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
() -> assertThat(server.getServer().getStats().getClients()).isEqualTo(initialClients));
}
+
+ @Test
+ public void keyspaceHitsStat_shouldIncrement_whenKeyAccessed() {
+ jedis.get(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceHitsStat_shouldNotIncrement_whenNonexistentKeyAccessed() {
+ jedis.get("Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ // TODO: Set doesn't work like native Redis!
+ @Test
+ public void keyspaceStats_setCommand_existingKey() {
+ jedis.set(EXISTING_STRING_KEY, "New_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ // TODO: Set doesn't work like native Redis!
+ @Test
+ public void keyspaceStats_setCommand_nonexistentKey() {
+ jedis.set("Another_Key", "Another_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getBitCommand_existingKey() {
+ jedis.getbit(EXISTING_STRING_KEY, 0);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getBitCommand_nonexistentKey() {
+ jedis.getbit("Nonexistent_Key", 0);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getRangeCommand_existingKey() {
+ jedis.getrange(EXISTING_STRING_KEY, 0, 1);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getRangeCommand_nonexistentKey() {
+ jedis.getrange("Nonexistent_Key", 0, 1);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getSetCommand_existingKey() {
+ jedis.getSet(EXISTING_STRING_KEY, "New_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getSetCommand_nonexistentKey() {
+ jedis.getSet("Nonexistent_Key", "FakeValue");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_strlenCommand_existingKey() {
+ jedis.strlen(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_strlenCommand_nonexistentKey() {
+ jedis.strlen(NONEXISTENT_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_mgetCommand() {
+ jedis.mget(EXISTING_STRING_KEY, "Nonexistent_Key");
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitopCommand() {
+ jedis.bitop(BitOP.AND, EXISTING_STRING_KEY, EXISTING_STRING_KEY, "Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 2);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitcountCommand_existingKey() {
+ jedis.bitcount(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitcountCommand_nonexistentKey() {
+ jedis.bitcount("Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitposCommand_existingKey() {
+ jedis.bitpos(EXISTING_STRING_KEY, true);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitposCommand_nonexistentKey() {
+ jedis.bitpos("Nonexistent_Key", true);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_hgetCommand_existingKey() {
+ jedis.hget(EXISTING_HASH_KEY, "Field1");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_hgetCommand_nonexistentKey() {
+ jedis.hget("Nonexistent_Hash", "Field1");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_smembersCommand_existingKey() {
+ jedis.smembers(EXISTING_SET_KEY_1);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_smembersCommand_nonexistentKey() {
+ jedis.smembers("Nonexistent_Set");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_sunionstoreCommand_existingKey() {
+ jedis.sunionstore("New_Set", EXISTING_SET_KEY_1, EXISTING_SET_KEY_2, "Nonexistent_Set");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 2);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
Review comment:
> why does this not cause two misses?
this is a weird interface- the first parameter is the set that the union will be stored in - so no hit or miss- just created. the 2 hits come from the existing keys. the miss comes form the non-existent set. let us know if that still seems weird and isn't addressed in a new story
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514486876
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -171,13 +180,92 @@ public void endCommand(RedisCommandType command, long start) {
}
public void addClient() {
+ connectionsReceived.incrementAndGet();
+ connectedClients.incrementAndGet();
stats.incLong(clientId, 1);
Review comment:
can likely be addressed in future changes to functionality
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513829391
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -58,6 +61,9 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
case "persistence":
result = getPersistenceSection();
break;
+ case "replication":
+ result = getReplicationSection();
+ break;
Review comment:
Seems like we also need to add the following cases: `clients`, `memory`, `stats`, and `keyspace`
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514478654
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/CommandHelper.java
##########
@@ -84,7 +84,13 @@ RedisData getRedisData(ByteArrayWrapper key, RedisData notFoundValue) {
}
RedisSet getRedisSet(ByteArrayWrapper key) {
- return checkSetType(getRedisData(key, NULL_REDIS_SET));
+ RedisData redisData = getRedisData(key, NULL_REDIS_SET);
+ if (redisData == NULL_REDIS_SET) {
+ redisStats.incKeyspaceMisses();
+ } else {
+ redisStats.incKeyspaceHits();
Review comment:
will address in future stories that are coming to investigate overall functionality of this piece
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514489966
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513829391
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -58,6 +61,9 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
case "persistence":
result = getPersistenceSection();
break;
+ case "replication":
+ result = getReplicationSection();
+ break;
Review comment:
Seems like we also need to add the following cases: `clients, memory, stats, and keyspace`
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514492694
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
story made for this
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514492350
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/SlowlogExecutor.java
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.server;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+// TODO: only exists for Redis monitoring software, maybe make functional someday?
+public class SlowlogExecutor extends AbstractExecutor {
Review comment:
done.
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513731184
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
Review comment:
I mentioned in another comment testing all the statistics, not just clients and keyspace hits and misses. We should also add additional tests/update existing tests in `AbstractInfoIntegrationTest`
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513757239
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisStats.java
##########
@@ -206,6 +294,7 @@ public void endExpiration(long start) {
stats.incLong(expirationTimeId, getTime() - start);
}
stats.incLong(expirationsId, 1);
+ expirations.incrementAndGet();
Review comment:
I don't think we are counting expirations properly. According to the Redis `INFO` docs (and also testing our version vs. Redis), it should be `the number of keys with an expiration` since we are using it in the `keyspace` section:
```
The keyspace section provides statistics on the main dictionary of each database. The statistics are the number of keys, and the number of keys with an expiration.
For each database, the following line is added:
dbXXX: keys=XXX,expires=XXX
```
This means that we should increment the count when any key has an expiration set and decrement it when that key no longer has an expiration (because it was persisted, expired on its own, was deleted, etc.). Right now, we only increment the count when a key expires, and we never decrement 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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518938551
##########
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:
done
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514490317
##########
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(
+ jedis.exists(toArray("doesNotExist"))).isEqualTo(0L);
Review comment:
will create separate formatting pr
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -169,7 +169,8 @@ public void initChannel(SocketChannel socketChannel) {
}
ChannelPipeline pipeline = socketChannel.pipeline();
addSSLIfEnabled(socketChannel, pipeline);
- pipeline.addLast(ByteToCommandDecoder.class.getSimpleName(), new ByteToCommandDecoder());
+ pipeline.addLast(ByteToCommandDecoder.class.getSimpleName(),
+ new ByteToCommandDecoder(redisStats));
Review comment:
will create separate formatting pr
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513731184
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
Review comment:
I mentioned in another comment that we should test all the statistics, not just clients and keyspace hits and misses. We should also add additional tests/update existing tests in `AbstractInfoIntegrationTest`
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514487901
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -58,6 +61,9 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
case "persistence":
result = getPersistenceSection();
break;
+ case "replication":
+ result = getReplicationSection();
+ break;
Review comment:
story made for this
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5678:
URL: https://github.com/apache/geode/pull/5678#issuecomment-718938111
for posterity, discussion around this PR lead to agreement that nativeRedisIntegration tests would be added to this PR, and additional functionality/refactors will be captured in additional stories.
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514497779
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
+ jedis.flushAll();
+ jedis.set(EXISTING_STRING_KEY, "A_Value");
+ jedis.hset(EXISTING_HASH_KEY, "Field1", "Value1");
+ jedis.sadd(EXISTING_SET_KEY_1, "m1", "m2", "m3");
+ jedis.sadd(EXISTING_SET_KEY_2, "m4", "m5", "m6");
+ initialKeyspaceHits = server.getServer().getStats().getKeyspaceHits();
+ initialKeyspaceMisses = server.getServer().getStats().getKeyspaceMisses();
+ }
+
@Test
- public void clientsStat_withConnectAndClose_isCorrect() throws InterruptedException {
+ public void clientsStat_withConnectAndClose_isCorrect() {
long initialClients = server.getServer().getStats().getClients();
Jedis jedis = new Jedis("localhost", server.getPort(), 10000000);
jedis.ping();
assertThat(server.getServer().getStats().getClients()).isEqualTo(initialClients + 1);
jedis.close();
- GeodeAwaitility.await().untilAsserted(
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
() -> assertThat(server.getServer().getStats().getClients()).isEqualTo(initialClients));
}
+
+ @Test
+ public void keyspaceHitsStat_shouldIncrement_whenKeyAccessed() {
+ jedis.get(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceHitsStat_shouldNotIncrement_whenNonexistentKeyAccessed() {
+ jedis.get("Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ // TODO: Set doesn't work like native Redis!
+ @Test
+ public void keyspaceStats_setCommand_existingKey() {
+ jedis.set(EXISTING_STRING_KEY, "New_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ // TODO: Set doesn't work like native Redis!
+ @Test
+ public void keyspaceStats_setCommand_nonexistentKey() {
+ jedis.set("Another_Key", "Another_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getBitCommand_existingKey() {
+ jedis.getbit(EXISTING_STRING_KEY, 0);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getBitCommand_nonexistentKey() {
+ jedis.getbit("Nonexistent_Key", 0);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getRangeCommand_existingKey() {
+ jedis.getrange(EXISTING_STRING_KEY, 0, 1);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getRangeCommand_nonexistentKey() {
+ jedis.getrange("Nonexistent_Key", 0, 1);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getSetCommand_existingKey() {
+ jedis.getSet(EXISTING_STRING_KEY, "New_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getSetCommand_nonexistentKey() {
+ jedis.getSet("Nonexistent_Key", "FakeValue");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_strlenCommand_existingKey() {
+ jedis.strlen(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_strlenCommand_nonexistentKey() {
+ jedis.strlen(NONEXISTENT_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_mgetCommand() {
+ jedis.mget(EXISTING_STRING_KEY, "Nonexistent_Key");
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitopCommand() {
+ jedis.bitop(BitOP.AND, EXISTING_STRING_KEY, EXISTING_STRING_KEY, "Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 2);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitcountCommand_existingKey() {
+ jedis.bitcount(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitcountCommand_nonexistentKey() {
+ jedis.bitcount("Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitposCommand_existingKey() {
+ jedis.bitpos(EXISTING_STRING_KEY, true);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitposCommand_nonexistentKey() {
+ jedis.bitpos("Nonexistent_Key", true);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_hgetCommand_existingKey() {
+ jedis.hget(EXISTING_HASH_KEY, "Field1");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_hgetCommand_nonexistentKey() {
+ jedis.hget("Nonexistent_Hash", "Field1");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_smembersCommand_existingKey() {
+ jedis.smembers(EXISTING_SET_KEY_1);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_smembersCommand_nonexistentKey() {
+ jedis.smembers("Nonexistent_Set");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_sunionstoreCommand_existingKey() {
+ jedis.sunionstore("New_Set", EXISTING_SET_KEY_1, EXISTING_SET_KEY_2, "Nonexistent_Set");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 2);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
Review comment:
this is a weird interface- the first parameter is the set that the union will be stored in - so no hit or miss- just created. the 2 hits come from the existing keys. the miss comes form the non-existent set. let us know if that still seems weird and isn't addressed in a new story
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514480813
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
Review comment:
will do
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514484657
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -169,7 +169,8 @@ public void initChannel(SocketChannel socketChannel) {
}
ChannelPipeline pipeline = socketChannel.pipeline();
addSSLIfEnabled(socketChannel, pipeline);
- pipeline.addLast(ByteToCommandDecoder.class.getSimpleName(), new ByteToCommandDecoder());
+ pipeline.addLast(ByteToCommandDecoder.class.getSimpleName(),
+ new ByteToCommandDecoder(redisStats));
Review comment:
will submit separate PR with format changes
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518940784
##########
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:
changed back
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r518934240
##########
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:
done
----------------------------------------------------------------
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513741057
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
+ jedis.flushAll();
+ jedis.set(EXISTING_STRING_KEY, "A_Value");
+ jedis.hset(EXISTING_HASH_KEY, "Field1", "Value1");
+ jedis.sadd(EXISTING_SET_KEY_1, "m1", "m2", "m3");
+ jedis.sadd(EXISTING_SET_KEY_2, "m4", "m5", "m6");
+ initialKeyspaceHits = server.getServer().getStats().getKeyspaceHits();
+ initialKeyspaceMisses = server.getServer().getStats().getKeyspaceMisses();
+ }
+
@Test
- public void clientsStat_withConnectAndClose_isCorrect() throws InterruptedException {
+ public void clientsStat_withConnectAndClose_isCorrect() {
long initialClients = server.getServer().getStats().getClients();
Jedis jedis = new Jedis("localhost", server.getPort(), 10000000);
jedis.ping();
assertThat(server.getServer().getStats().getClients()).isEqualTo(initialClients + 1);
jedis.close();
- GeodeAwaitility.await().untilAsserted(
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
() -> assertThat(server.getServer().getStats().getClients()).isEqualTo(initialClients));
}
+
+ @Test
+ public void keyspaceHitsStat_shouldIncrement_whenKeyAccessed() {
+ jedis.get(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceHitsStat_shouldNotIncrement_whenNonexistentKeyAccessed() {
+ jedis.get("Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ // TODO: Set doesn't work like native Redis!
+ @Test
+ public void keyspaceStats_setCommand_existingKey() {
+ jedis.set(EXISTING_STRING_KEY, "New_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ // TODO: Set doesn't work like native Redis!
+ @Test
+ public void keyspaceStats_setCommand_nonexistentKey() {
+ jedis.set("Another_Key", "Another_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getBitCommand_existingKey() {
+ jedis.getbit(EXISTING_STRING_KEY, 0);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getBitCommand_nonexistentKey() {
+ jedis.getbit("Nonexistent_Key", 0);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getRangeCommand_existingKey() {
+ jedis.getrange(EXISTING_STRING_KEY, 0, 1);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getRangeCommand_nonexistentKey() {
+ jedis.getrange("Nonexistent_Key", 0, 1);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getSetCommand_existingKey() {
+ jedis.getSet(EXISTING_STRING_KEY, "New_Value");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_getSetCommand_nonexistentKey() {
+ jedis.getSet("Nonexistent_Key", "FakeValue");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_strlenCommand_existingKey() {
+ jedis.strlen(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_strlenCommand_nonexistentKey() {
+ jedis.strlen(NONEXISTENT_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_mgetCommand() {
+ jedis.mget(EXISTING_STRING_KEY, "Nonexistent_Key");
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitopCommand() {
+ jedis.bitop(BitOP.AND, EXISTING_STRING_KEY, EXISTING_STRING_KEY, "Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 2);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitcountCommand_existingKey() {
+ jedis.bitcount(EXISTING_STRING_KEY);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitcountCommand_nonexistentKey() {
+ jedis.bitcount("Nonexistent_Key");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitposCommand_existingKey() {
+ jedis.bitpos(EXISTING_STRING_KEY, true);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_bitposCommand_nonexistentKey() {
+ jedis.bitpos("Nonexistent_Key", true);
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_hgetCommand_existingKey() {
+ jedis.hget(EXISTING_HASH_KEY, "Field1");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_hgetCommand_nonexistentKey() {
+ jedis.hget("Nonexistent_Hash", "Field1");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_smembersCommand_existingKey() {
+ jedis.smembers(EXISTING_SET_KEY_1);
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 1);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_smembersCommand_nonexistentKey() {
+ jedis.smembers("Nonexistent_Set");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
+ });
+ }
+
+ @Test
+ public void keyspaceStats_sunionstoreCommand_existingKey() {
+ jedis.sunionstore("New_Set", EXISTING_SET_KEY_1, EXISTING_SET_KEY_2, "Nonexistent_Set");
+
+ jedis.close();
+ GeodeAwaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ assertThat(server.getServer().getStats().getKeyspaceHits())
+ .isEqualTo(initialKeyspaceHits + 2);
+ assertThat(server.getServer().getStats().getKeyspaceMisses())
+ .isEqualTo(initialKeyspaceMisses + 1);
Review comment:
why does this not cause two misses?
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513731184
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
Review comment:
I mentioned above testing all the statistics, not just clients and keyspace hits and misses. We should also add additional tests/update existing tests in `AbstractInfoIntegrationTest`
----------------------------------------------------------------
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] jhutchison commented on pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5678:
URL: https://github.com/apache/geode/pull/5678#issuecomment-718792965
> It seems like there are some things to consider here that we could potentially discuss as a team or investigate more. Mostly around `keyspaceHits` and `keyspaceMisses`. Do we want to count those in the same way Redis does? One potential use case/argument for counting them the way Redis does is that a user might want to know how many times they tried to read something that wasn't in the database (it wouldn't really matter if they did a `SADD`, `SET`, or some other create operation and the key was null, for example, because it just gets added if it's not there). But I think you could make an argument either way.
>
> Another thing to note is that our `total_commands_processed` is not currently the same as Redis, since Redis does not count unknown commands as processed commands, but we do.
Yeah, let's talk this over as a team. I think there were some decisions made early on in this workflow that maybe we need to think through together. reverted to draft fro now.
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513738053
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/SelectExecutor.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ *
+ */
+package org.apache.geode.redis.internal.executor.connection;
+
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class SelectExecutor extends AbstractExecutor {
Review comment:
Not sure if we want to add the `SelectExecutor` in this PR, since there's another PR that adds it. If we do add it in this PR, we need to remove `SELECT` from the list of unimplemented commands and add it to the list of unsupported commands in `SupportedCommandsJUnitTest`.
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513596225
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/RedisStatsIntegrationTest.java
##########
@@ -16,28 +16,469 @@
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
+import redis.clients.jedis.BitOP;
import redis.clients.jedis.Jedis;
import org.apache.geode.redis.GeodeRedisServerRule;
import org.apache.geode.test.awaitility.GeodeAwaitility;
public class RedisStatsIntegrationTest {
+ public static final String EXISTING_HASH_KEY = "Existing_Hash";
+ public static final String EXISTING_STRING_KEY = "Existing_String";
+ public static final String EXISTING_SET_KEY_1 = "Existing_Set_1";
+ public static final String EXISTING_SET_KEY_2 = "Existing_Set_2";
+ public static final String NONEXISTENT_KEY = "Nonexistent_Key";
+ Jedis jedis;
+ long initialKeyspaceHits;
+ long initialKeyspaceMisses;
@ClassRule
public static GeodeRedisServerRule server = new GeodeRedisServerRule();
+ @Before
+ public void setup() {
+ jedis = new Jedis("localhost", server.getPort(), 10000000);
+ jedis.flushAll();
+ jedis.set(EXISTING_STRING_KEY, "A_Value");
+ jedis.hset(EXISTING_HASH_KEY, "Field1", "Value1");
+ jedis.sadd(EXISTING_SET_KEY_1, "m1", "m2", "m3");
+ jedis.sadd(EXISTING_SET_KEY_2, "m4", "m5", "m6");
+ initialKeyspaceHits = server.getServer().getStats().getKeyspaceHits();
+ initialKeyspaceMisses = server.getServer().getStats().getKeyspaceMisses();
+ }
+
@Test
- public void clientsStat_withConnectAndClose_isCorrect() throws InterruptedException {
+ public void clientsStat_withConnectAndClose_isCorrect() {
long initialClients = server.getServer().getStats().getClients();
Jedis jedis = new Jedis("localhost", server.getPort(), 10000000);
Review comment:
I don't think this Jedis instance is necessary since we've already created one in the test setup.
----------------------------------------------------------------
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
[GitHub] [geode] sabbey37 commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r513658138
##########
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();
+ }
Review comment:
Similar to the comment in the `CommandHelper` class, if we decide to increment the hits/misses this way, we could consolidate all of these and just have the if/else in the `getRedisData` method.
----------------------------------------------------------------
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
[GitHub] [geode] jhutchison commented on a change in pull request #5678: GEODE-8663: update Redis Info command To include additional statistics
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5678:
URL: https://github.com/apache/geode/pull/5678#discussion_r514489536
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -58,6 +61,9 @@ private String getSpecifiedSection(ExecutionHandlerContext context,
case "persistence":
result = getPersistenceSection();
break;
+ case "replication":
+ result = getReplicationSection();
+ break;
Review comment:
story added to implement this.
----------------------------------------------------------------
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