You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by je...@apache.org on 2021/02/01 21:01:45 UTC
[geode] branch develop updated: GEODE-8897: Improve Redis
INCRBYFLOAT output accuracy for very large values (#5980)
This is an automated email from the ASF dual-hosted git repository.
jensdeppe pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new c537109 GEODE-8897: Improve Redis INCRBYFLOAT output accuracy for very large values (#5980)
c537109 is described below
commit c537109603f66b04c6533f6c971b40e22113ec12
Author: Jens Deppe <jd...@pivotal.io>
AuthorDate: Mon Feb 1 13:00:39 2021 -0800
GEODE-8897: Improve Redis INCRBYFLOAT output accuracy for very large values (#5980)
- Switch to using BigDecimal instead of double for increment value.
- Also mark INCRBYFLOAT as supported.
- Add concurrency tests.
---
.../string/AbstractIncrByFloatIntegrationTest.java | 43 ++++++++++++++++------
.../geode/redis/internal/RedisCommandType.java | 2 +-
.../geode/redis/internal/data/NullRedisString.java | 7 ++--
.../geode/redis/internal/data/RedisString.java | 22 +++++------
.../data/RedisStringCommandsFunctionExecutor.java | 3 +-
.../redis/internal/executor/CommandFunction.java | 3 +-
.../redis/internal/executor/RedisResponse.java | 5 ++-
.../executor/string/IncrByFloatExecutor.java | 9 +++--
.../executor/string/RedisStringCommands.java | 3 +-
.../string/RedisStringCommandsFunctionInvoker.java | 3 +-
.../apache/geode/redis/internal/netty/Coder.java | 13 ++++++-
.../redis/internal/SupportedCommandsJUnitTest.java | 2 +-
.../geode/redis/internal/data/RedisStringTest.java | 7 ++--
13 files changed, 78 insertions(+), 44 deletions(-)
diff --git a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrByFloatIntegrationTest.java b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrByFloatIntegrationTest.java
index 1182c23..8906f42 100755
--- a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrByFloatIntegrationTest.java
+++ b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractIncrByFloatIntegrationTest.java
@@ -18,14 +18,16 @@ import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.math.BigDecimal;
+import java.util.Random;
+import java.util.concurrent.atomic.AtomicReference;
import org.junit.After;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Test;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.Protocol;
+import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.test.awaitility.GeodeAwaitility;
import org.apache.geode.test.dunit.rules.RedisPortSupplier;
@@ -150,17 +152,6 @@ public abstract class AbstractIncrByFloatIntegrationTest implements RedisPortSup
}
@Test
- public void testIncrByFloat_whenIncrWillOverflowProducesCorrectError() {
- BigDecimal longDouble = new BigDecimal("1.1E+4932");
- jedis.set("number", longDouble.toPlainString());
-
- assertThatThrownBy(
- () -> jedis.sendCommand(Protocol.Command.INCRBYFLOAT, "number", longDouble.toString()))
- .hasMessage("ERR increment would produce NaN or Infinity");
- }
-
- @Test
- @Ignore("GEODE-8624: Improve INCRBYFLOAT accuracy for very large values")
public void testIncrByFloat_withReallyBigNumbers() {
// max unsigned long long - 1
BigDecimal biggy = new BigDecimal("18446744073709551614");
@@ -172,4 +163,32 @@ public abstract class AbstractIncrByFloatIntegrationTest implements RedisPortSup
assertThat(result.toPlainString()).isEqualTo(biggy.add(BigDecimal.ONE).toPlainString());
}
+
+ @Test
+ public void testConcurrentIncrByFloat_performsAllIncrByFloats() {
+ String key = "key";
+ Random random = new Random();
+ Jedis jedis2 = new Jedis("localhost", getPort(), JEDIS_TIMEOUT);
+
+ AtomicReference<BigDecimal> expectedValue = new AtomicReference<>();
+ expectedValue.set(new BigDecimal(0));
+
+ jedis.set(key, "0");
+
+ new ConcurrentLoopingThreads(1000,
+ (i) -> {
+ BigDecimal increment = BigDecimal.valueOf(random.nextInt(37));
+ expectedValue.getAndUpdate(x -> x.add(increment));
+ jedis.sendCommand(Protocol.Command.INCRBYFLOAT, key, increment.toPlainString());
+ },
+ (i) -> {
+ BigDecimal increment = BigDecimal.valueOf(random.nextInt(37));
+ expectedValue.getAndUpdate(x -> x.add(increment));
+ jedis2.sendCommand(Protocol.Command.INCRBYFLOAT, key, increment.toPlainString());
+ }).run();
+
+ assertThat(new BigDecimal(jedis.get(key))).isEqualTo(expectedValue.get());
+
+ jedis2.close();
+ }
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
index 7948185..771dddd 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
@@ -151,6 +151,7 @@ public enum RedisCommandType {
APPEND(new AppendExecutor(), SUPPORTED, new ExactParameterRequirements(3)),
GET(new GetExecutor(), SUPPORTED, new ExactParameterRequirements(2)),
+ INCRBYFLOAT(new IncrByFloatExecutor(), SUPPORTED, new ExactParameterRequirements(3)),
SET(new SetExecutor(), SUPPORTED, new MinimumParameterRequirements(3)),
/************* Hashes *****************/
@@ -219,7 +220,6 @@ public enum RedisCommandType {
GETSET(new GetSetExecutor(), UNSUPPORTED, new ExactParameterRequirements(3)),
INCR(new IncrExecutor(), UNSUPPORTED, new ExactParameterRequirements(2)),
INCRBY(new IncrByExecutor(), UNSUPPORTED, new ExactParameterRequirements(3)),
- INCRBYFLOAT(new IncrByFloatExecutor(), UNSUPPORTED, new ExactParameterRequirements(3)),
MGET(new MGetExecutor(), UNSUPPORTED, new MinimumParameterRequirements(2)),
MSET(new MSetExecutor(), UNSUPPORTED,
new MinimumParameterRequirements(3).and(new OddParameterRequirements())),
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisString.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisString.java
index 867797b..89e3fa6 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisString.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisString.java
@@ -16,6 +16,7 @@
package org.apache.geode.redis.internal.data;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.List;
@@ -109,9 +110,9 @@ public class NullRedisString extends RedisString {
}
@Override
- public double incrbyfloat(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
- double increment) throws NumberFormatException, ArithmeticException {
- byte[] newValue = Coder.doubleToBytes(increment);
+ public BigDecimal incrbyfloat(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
+ BigDecimal increment) throws NumberFormatException, ArithmeticException {
+ byte[] newValue = Coder.bigDecimalToBytes(increment);
region.put(key, new RedisString(new ByteArrayWrapper(newValue)));
return increment;
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
index b75cd71..fc14814 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
@@ -19,6 +19,7 @@ package org.apache.geode.redis.internal.data;
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import java.math.BigDecimal;
import java.util.Arrays;
import java.util.Objects;
@@ -88,18 +89,16 @@ public class RedisString extends AbstractRedisData {
return longValue;
}
- public double incrbyfloat(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
- double increment)
+ public BigDecimal incrbyfloat(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
+ BigDecimal increment)
throws NumberFormatException, ArithmeticException {
- double doubleValue = parseValueAsDouble();
- doubleValue += increment;
- if (Double.isNaN(doubleValue) || Double.isInfinite(doubleValue)) {
- throw new ArithmeticException(RedisConstants.ERROR_NAN_OR_INFINITY);
- }
- valueSetBytes(Coder.doubleToBytes(doubleValue));
+ BigDecimal bigDecimalValue = parseValueAsBigDecimal();
+ bigDecimalValue = bigDecimalValue.add(increment);
+ valueSetBytes(Coder.bigDecimalToBytes(bigDecimalValue));
+
// numeric strings are short so no need to use delta
region.put(key, this);
- return doubleValue;
+ return bigDecimalValue;
}
public long decrby(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
@@ -136,17 +135,16 @@ public class RedisString extends AbstractRedisData {
}
}
- private double parseValueAsDouble() {
+ private BigDecimal parseValueAsBigDecimal() {
String valueString = value.toString();
if (valueString.contains(" ")) {
throw new NumberFormatException(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
}
try {
- return Coder.stringToDouble(valueString);
+ return new BigDecimal(valueString);
} catch (NumberFormatException e) {
throw new NumberFormatException(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
}
-
}
public ByteArrayWrapper getrange(long start, long end) {
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor.java
index f162bc0..515e30d 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor.java
@@ -17,6 +17,7 @@ package org.apache.geode.redis.internal.data;
import static org.apache.geode.redis.internal.data.RedisString.NULL_REDIS_STRING;
+import java.math.BigDecimal;
import java.util.List;
import org.apache.geode.redis.internal.executor.string.RedisStringCommands;
@@ -85,7 +86,7 @@ public class RedisStringCommandsFunctionExecutor extends RedisDataCommandsFuncti
}
@Override
- public double incrbyfloat(ByteArrayWrapper key, double increment) {
+ public BigDecimal incrbyfloat(ByteArrayWrapper key, BigDecimal increment) {
return stripedExecute(key,
() -> getRedisString(key, false)
.incrbyfloat(getRegion(), key, increment));
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java
index 5d0a08c..81b5ef1 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java
@@ -16,6 +16,7 @@
package org.apache.geode.redis.internal.executor;
+import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.ArrayList;
import java.util.List;
@@ -169,7 +170,7 @@ public class CommandFunction extends SingleResultRedisFunction {
return stringCommands.incrby(key, increment);
}
case INCRBYFLOAT: {
- double increment = (double) args[1];
+ BigDecimal increment = (BigDecimal) args[1];
return stringCommands.incrbyfloat(key, increment);
}
case DECRBY: {
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisResponse.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisResponse.java
index 7c96f3c..db50bc1 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisResponse.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisResponse.java
@@ -16,6 +16,7 @@
package org.apache.geode.redis.internal.executor;
+import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.charset.Charset;
import java.util.ArrayList;
@@ -143,8 +144,8 @@ public class RedisResponse {
return encode(new UnpooledByteBufAllocator(false)).toString(Charset.defaultCharset());
}
- public static RedisResponse doubleValue(double numericValue) {
- return new RedisResponse((bba) -> Coder.getDoubleResponse(bba, numericValue));
+ public static RedisResponse bigDecimal(BigDecimal numericValue) {
+ return new RedisResponse((bba) -> Coder.getBigDecimalResponse(bba, numericValue));
}
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/IncrByFloatExecutor.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/IncrByFloatExecutor.java
index fc22343..79995f2 100755
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/IncrByFloatExecutor.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/IncrByFloatExecutor.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.redis.internal.executor.string;
+import java.math.BigDecimal;
import java.util.List;
import java.util.regex.Pattern;
@@ -43,15 +44,15 @@ public class IncrByFloatExecutor extends StringExecutor {
return RedisResponse.error(RedisConstants.ERROR_NAN_OR_INFINITY);
}
- double increment;
+ BigDecimal increment;
try {
- increment = Coder.bytesToDouble(incrArray);
+ increment = Coder.bytesToBigDecimal(incrArray);
} catch (NumberFormatException e) {
return RedisResponse.error(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
}
- double result = stringCommands.incrbyfloat(key, increment);
+ BigDecimal result = stringCommands.incrbyfloat(key, increment);
- return RedisResponse.doubleValue(result);
+ return RedisResponse.bigDecimal(result);
}
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommands.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommands.java
index a58dd4e..f69822d 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommands.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommands.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.redis.internal.executor.string;
+import java.math.BigDecimal;
import java.util.List;
import org.apache.geode.redis.internal.data.ByteArrayWrapper;
@@ -47,7 +48,7 @@ public interface RedisStringCommands {
int setbit(ByteArrayWrapper key, long offset, int value);
- double incrbyfloat(ByteArrayWrapper key, double increment);
+ BigDecimal incrbyfloat(ByteArrayWrapper key, BigDecimal increment);
int bitop(String operation, ByteArrayWrapper destKey, List<ByteArrayWrapper> sources);
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommandsFunctionInvoker.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommandsFunctionInvoker.java
index c2211c8..a588161 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommandsFunctionInvoker.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/string/RedisStringCommandsFunctionInvoker.java
@@ -33,6 +33,7 @@ import static org.apache.geode.redis.internal.RedisCommandType.SETBIT;
import static org.apache.geode.redis.internal.RedisCommandType.SETRANGE;
import static org.apache.geode.redis.internal.RedisCommandType.STRLEN;
+import java.math.BigDecimal;
import java.util.List;
import org.apache.geode.cache.Region;
@@ -123,7 +124,7 @@ public class RedisStringCommandsFunctionInvoker extends RedisCommandsFunctionInv
}
@Override
- public double incrbyfloat(ByteArrayWrapper key, double increment) {
+ public BigDecimal incrbyfloat(ByteArrayWrapper key, BigDecimal increment) {
return invokeCommandFunction(key, INCRBYFLOAT, increment);
}
diff --git a/geode-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java b/geode-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
index 79a609f..0f62e1f 100644
--- a/geode-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
+++ b/geode-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
@@ -15,6 +15,7 @@
*/
package org.apache.geode.redis.internal.netty;
+import java.math.BigDecimal;
import java.math.BigInteger;
import java.text.DecimalFormat;
import java.util.Collection;
@@ -312,9 +313,9 @@ public class Coder {
return response;
}
- public static ByteBuf getDoubleResponse(ByteBufAllocator alloc, double d) {
+ public static ByteBuf getBigDecimalResponse(ByteBufAllocator alloc, BigDecimal b) {
ByteBuf response = alloc.buffer();
- writeStringResponse(response, doubleToBytes(d));
+ writeStringResponse(response, bigDecimalToBytes(b));
return response;
}
@@ -370,6 +371,14 @@ public class Coder {
return stringToBytes(doubleToString(d));
}
+ public static byte[] bigDecimalToBytes(BigDecimal b) {
+ return stringToBytes(b.toPlainString());
+ }
+
+ public static BigDecimal bytesToBigDecimal(byte[] bytes) {
+ return new BigDecimal(bytesToString(bytes));
+ }
+
public static int bytesToInt(byte[] bytes) {
return Integer.parseInt(bytesToString(bytes));
}
diff --git a/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java b/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
index 7bdf157..2a54fc4 100644
--- a/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
+++ b/geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
@@ -45,6 +45,7 @@ public class SupportedCommandsJUnitTest {
"HMGET",
"HSTRLEN",
"HVALS",
+ "INCRBYFLOAT",
"KEYS",
"PERSIST",
"PEXPIRE",
@@ -88,7 +89,6 @@ public class SupportedCommandsJUnitTest {
"HSCAN",
"INCR",
"INCRBY",
- "INCRBYFLOAT",
"INFO",
"MGET",
"MSET",
diff --git a/geode-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java b/geode-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
index e214354..73f3302 100644
--- a/geode-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
+++ b/geode-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
@@ -21,6 +21,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.mock;
import java.io.IOException;
+import java.math.BigDecimal;
import org.junit.BeforeClass;
import org.junit.Test;
@@ -193,7 +194,7 @@ public class RedisStringTest {
Region<ByteArrayWrapper, RedisData> region = mock(Region.class);
ByteArrayWrapper byteArrayWrapper = new ByteArrayWrapper(new byte[] {'1', '0', ' ', '1'});
RedisString string = new RedisString(byteArrayWrapper);
- assertThatThrownBy(() -> string.incrbyfloat(region, byteArrayWrapper, 1.1))
+ assertThatThrownBy(() -> string.incrbyfloat(region, byteArrayWrapper, new BigDecimal("1.1")))
.isInstanceOf(NumberFormatException.class);
}
@@ -204,8 +205,8 @@ public class RedisStringTest {
Region<ByteArrayWrapper, RedisData> region = mock(Region.class);
ByteArrayWrapper byteArrayWrapper = new ByteArrayWrapper(new byte[] {'1', '0'});
RedisString string = new RedisString(byteArrayWrapper);
- string.incrbyfloat(region, byteArrayWrapper, 2.20);
- assertThat(string.get().toString()).isEqualTo("12.2");
+ string.incrbyfloat(region, byteArrayWrapper, new BigDecimal("2.20"));
+ assertThat(string.get().toString()).isEqualTo("12.20");
}
@Test