[GitHub] [geode] sabbey37 commented on a change in pull request #5654: add integration tests for strlen

[GitBox <gi...@apache.org>]

sabbey37 commented on a change in pull request #5654:
URL: https://github.com/apache/geode/pull/5654#discussion_r511997258



##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrIntegrationTest.java
##########
@@ -104,7 +104,7 @@ public void testIncr_whenWrongType_shouldReturnError() {
     try {
       jedis.incr(key);
     } catch (JedisDataException e) {
-      assertThat(e.getMessage()).contains(RedisConstants.ERROR_NOT_INTEGER);
+      assertThat(e.getMessage()).contains("out of range");

Review comment:
       Could we change this back to the `ERROR_NOT_INTEGER` constant? Geode Redis should return the same errors as Redis.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -42,6 +42,7 @@
   public static final String ERROR_WRONG_TYPE =
       "Operation against a key holding the wrong kind of value";
   public static final String ERROR_NOT_INTEGER = "value is not an integer or out of range";
+  public static final String ERROR_NOT_LONG = "value is not a long or out of range";

Review comment:
       I don't think Redis ever returns this error.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -134,7 +132,7 @@ private long parseValueAsLong() {
     try {
       return Long.parseLong(value.toString());
     } catch (NumberFormatException ex) {
-      throw new NumberFormatException(RedisConstants.ERROR_NOT_INTEGER);
+      throw new NumberFormatException(RedisConstants.ERROR_NOT_LONG);

Review comment:
       I definitely understand your thinking here.  However, we want to match the errors returned by Redis, which is why we are returning the not integer error.  We had to use a Java long here instead of an integer because the integer's storage capacity was not large enough, according to the Redis [INCR docs](https://redis.io/commands/incr) `the string stored at the key is interpreted as a base-10 64 bit signed integer`.




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