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:
[email protected]
