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