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 2021/06/01 23:26:05 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #6534: GEODE-9306: support ZINCRBY command
DonalEvans commented on a change in pull request #6534:
URL: https://github.com/apache/geode/pull/6534#discussion_r643534867
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -228,6 +230,26 @@ private void validateScoreIsDouble(byte[] score) {
return members.get(member);
}
+ byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[] increment,
+ byte[] member) {
+ byte[] score = members.get(member);
+ byte[] incr = Coder.doubleToBytes(
+ processIncrement(Coder.bytesToString(increment).toLowerCase()));
+
+ if (score != null) {
+ incr = Coder.doubleToBytes(Coder.bytesToDouble(score) + Coder.bytesToDouble(incr));
Review comment:
This approach seems a little expensive, given that each call to `bytesToDouble()` is actually a `bytesToString()` followed by a `stringToDouble()`. I think it might be better to assign `incr` as a double on line 236, then only convert `score` to double on this line, and finally create a variable `incrBytes` to use in the rest of the method. Making these changes will save one call each to `bytesToString()`, `doubleToString()`, `stringToBytes()` and `stringToDouble()`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java
##########
@@ -49,4 +51,19 @@ long zadd(Region<RedisKey, RedisData> region, RedisKey key, List<byte[]> members
byte[] zscore(byte[] member) {
return null;
}
+
+ @Override
+ byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[] increment,
+ byte[] member) {
+ byte[] incr = Coder.doubleToBytes(processIncrement(Coder.bytesToString(increment)));
Review comment:
In this method, since no arithmetic needs to be done on the value of `increment`, I think it would be more efficient to perform a check that the value can be parsed to a double in `processIncrement()`, but just return a String, since then we're not converting from a String to a double and then immediately back again (as part of the `doubleToBytes()` call).
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongNumberOfArguments() {
+ jedis.zadd(KEY, 0.0, MEMBER);
+ assertExactNumberOfArgs(jedis, Protocol.Command.ZINCRBY, 3);
+ }
+
+ @Test
+ public void shouldError_givenNonFloatIncrement() {
+ String nonFloatIncrement = "q";
+ jedis.zadd(KEY, 1.0, MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nonFloatIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ @Test
+ public void shouldError_givenNaNIncrement() {
+ String nanIncrement = "NaN";
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nanIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ /************* infinity *************/
+ @Test
+ public void shouldSetScoreToInfinity_ifIncrementWouldExceedMaxValue() {
+ double increment = Double.MAX_VALUE / 2;
+
+ jedis.zadd(STRING_KEY, Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, -Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, -increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void shouldSetScoreToInfinity_givenInfiniteIncrement() {
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, POSITIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, NEGATIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfPositiveInfinity() {
Review comment:
The name of this test is a little confusing to me. Is it intended to test that `incrby` called on a null set should behave as expected? Because if so, the call to `zadd` should be omitted, since that creates the set, meaning that the code in `NullRedisSortedSet.zincrby()` is never executed.
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongNumberOfArguments() {
+ jedis.zadd(KEY, 0.0, MEMBER);
+ assertExactNumberOfArgs(jedis, Protocol.Command.ZINCRBY, 3);
+ }
+
+ @Test
+ public void shouldError_givenNonFloatIncrement() {
+ String nonFloatIncrement = "q";
+ jedis.zadd(KEY, 1.0, MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nonFloatIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ @Test
+ public void shouldError_givenNaNIncrement() {
+ String nanIncrement = "NaN";
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nanIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ /************* infinity *************/
+ @Test
+ public void shouldSetScoreToInfinity_ifIncrementWouldExceedMaxValue() {
+ double increment = Double.MAX_VALUE / 2;
+
+ jedis.zadd(STRING_KEY, Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, -Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, -increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void shouldSetScoreToInfinity_givenInfiniteIncrement() {
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, POSITIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, NEGATIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
+
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfNegativeInfinity() {
Review comment:
See above comment on test name/intended behaviour related to `NullRedisSortedSet`.
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongNumberOfArguments() {
+ jedis.zadd(KEY, 0.0, MEMBER);
+ assertExactNumberOfArgs(jedis, Protocol.Command.ZINCRBY, 3);
+ }
+
+ @Test
+ public void shouldError_givenNonFloatIncrement() {
+ String nonFloatIncrement = "q";
+ jedis.zadd(KEY, 1.0, MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nonFloatIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ @Test
+ public void shouldError_givenNaNIncrement() {
+ String nanIncrement = "NaN";
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nanIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ /************* infinity *************/
+ @Test
+ public void shouldSetScoreToInfinity_ifIncrementWouldExceedMaxValue() {
+ double increment = Double.MAX_VALUE / 2;
+
+ jedis.zadd(STRING_KEY, Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, -Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, -increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void shouldSetScoreToInfinity_givenInfiniteIncrement() {
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, POSITIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, NEGATIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
+
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfNegativeInfinity() {
+ final double increment = NEGATIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
Review comment:
Another call to `getBytes()` that could be replaced with `Coder.stringToBytes()`.
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
Review comment:
Can these calls to `getBytes()` be replaced with calls to `Coder.stringToBytes()` please? There's an open ticket to make sure that all the String conversions are using the same encoding, so making sure we're not adding more unqualified conversions will save work later. Also, since the byte array constants are based on the String constants, could they be defined as such:
```
final byte[] KEY = Coder.stringToBytes(STRING_KEY);
```
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongNumberOfArguments() {
+ jedis.zadd(KEY, 0.0, MEMBER);
+ assertExactNumberOfArgs(jedis, Protocol.Command.ZINCRBY, 3);
+ }
+
+ @Test
+ public void shouldError_givenNonFloatIncrement() {
+ String nonFloatIncrement = "q";
+ jedis.zadd(KEY, 1.0, MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nonFloatIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ @Test
+ public void shouldError_givenNaNIncrement() {
+ String nanIncrement = "NaN";
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nanIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ /************* infinity *************/
+ @Test
+ public void shouldSetScoreToInfinity_ifIncrementWouldExceedMaxValue() {
+ double increment = Double.MAX_VALUE / 2;
+
+ jedis.zadd(STRING_KEY, Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, -Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, -increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void shouldSetScoreToInfinity_givenInfiniteIncrement() {
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, POSITIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, NEGATIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
+
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfNegativeInfinity() {
+ final double increment = NEGATIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void shouldCreateNonExistentKeyWithInfiniteScore_whenIncrementIsPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void shouldCreateNonExistentKeyWithInfiniteScore_whenIncrementIsNegativeInfinity() {
+ final double increment = NEGATIVE_INFINITY;
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void scoreOfPositiveInfinityShouldRemainInfinite_whenIncrementingOrDecrementing() {
+ jedis.zadd(KEY, POSITIVE_INFINITY, MEMBER);
+
+ jedis.zincrby(KEY, 100.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zincrby(KEY, -1000.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(POSITIVE_INFINITY);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void scoreOfNegativeInfinityShouldRemainInfinite_whenIncrementingOrDecrementing() {
+ jedis.zadd(KEY, NEGATIVE_INFINITY, MEMBER);
+
+ jedis.zincrby(KEY, 100.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zincrby(KEY, -1000.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void incrementingScoreOfInfinityByNegativeInfinity_throwsNaNError() {
+ jedis.zadd(KEY, POSITIVE_INFINITY, MEMBER);
+
+ assertThatThrownBy(() -> jedis.zincrby(KEY, NEGATIVE_INFINITY, MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_OPERATION_PRODUCED_NAN);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void incrementingScoreOfNegativeInfinityByPositiveInfinity_throwsNaNError() {
+ jedis.zadd(KEY, NEGATIVE_INFINITY, MEMBER);
+
+ assertThatThrownBy(() -> jedis.zincrby(KEY, POSITIVE_INFINITY, MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_OPERATION_PRODUCED_NAN);
+ }
+
+ /************* key or member does not exist *************/
+ @Test
+ public void shouldCreateNewKey_whenIncrementedKeyDoesNotExist() {
Review comment:
This test and the one below it are very similar in what they're testing to the `shouldCreateNonExistentKeyWithInfiniteScore_whenIncrementIs*` tests above. Would it be possible to create parameterized versions of the tests that pass +infinity, -infinity and a finite increment and assert the expected behaviour, rather than having three separate tests?
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZIncrByExecutor.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.sortedset;
+
+
+
+import java.util.Iterator;
+import java.util.List;
+
+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 ZIncrByExecutor extends AbstractExecutor {
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+ RedisSortedSetCommands redisSortedSetCommands = context.getRedisSortedSetCommands();
+ List<byte[]> commandElements = command.getProcessedCommand();
+ Iterator<byte[]> commandIterator = commandElements.iterator();
+
+ skipCommandAndKey(commandIterator);
+ byte[] increment = commandIterator.next();
+ byte[] member = commandIterator.next();
Review comment:
Rather than using an iterator here, it might be better to just directly access the list, given that we know the contents will always be `{command, key, increment, member}`.
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongNumberOfArguments() {
+ jedis.zadd(KEY, 0.0, MEMBER);
+ assertExactNumberOfArgs(jedis, Protocol.Command.ZINCRBY, 3);
+ }
+
+ @Test
+ public void shouldError_givenNonFloatIncrement() {
+ String nonFloatIncrement = "q";
+ jedis.zadd(KEY, 1.0, MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nonFloatIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ @Test
+ public void shouldError_givenNaNIncrement() {
+ String nanIncrement = "NaN";
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nanIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ /************* infinity *************/
+ @Test
+ public void shouldSetScoreToInfinity_ifIncrementWouldExceedMaxValue() {
+ double increment = Double.MAX_VALUE / 2;
+
+ jedis.zadd(STRING_KEY, Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, -Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, -increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void shouldSetScoreToInfinity_givenInfiniteIncrement() {
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, POSITIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, NEGATIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
Review comment:
Another call to `getBytes()` that could be replaced with `Coder.stringToBytes()`.
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
Review comment:
"WRONGTYPE" is defined as a constant in `Coder` as `Coder.wrongType` so maybe that should be used here. (Also, incidentally, that constant should probably be renamed to follow the all-caps constant naming convention.)
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/RedisResponse.java
##########
@@ -71,6 +71,11 @@ public static RedisResponse string(byte[] byteArray) {
return new RedisResponse((buffer) -> Coder.getSimpleStringResponse(buffer, byteArray));
}
+ public static RedisResponse string(double doubleValue) {
Review comment:
This method doesn't seem to be used anywhere, is it necessary?
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZIncrByIntegrationTest.java
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.sortedset;
+
+import static java.lang.Double.NEGATIVE_INFINITY;
+import static java.lang.Double.POSITIVE_INFINITY;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.concurrent.ThreadLocalRandom;
+
+import com.google.common.util.concurrent.AtomicDouble;
+import org.assertj.core.data.Offset;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractZIncrByIntegrationTest implements RedisIntegrationTest {
+ JedisCluster jedis;
+ final byte[] KEY = "key".getBytes();
+ final String STRING_KEY = "key";
+ final byte[] MEMBER = "member".getBytes();
+ final String STRING_MEMBER = "member";
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ /************* errors *************/
+ @Test
+ public void shouldError_givenWrongKeyType() {
+ jedis.set(STRING_KEY, "value");
+ assertThatThrownBy(
+ () -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "1", "member"))
+ .hasMessageContaining("WRONGTYPE " + RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @Test
+ public void shouldError_givenWrongNumberOfArguments() {
+ jedis.zadd(KEY, 0.0, MEMBER);
+ assertExactNumberOfArgs(jedis, Protocol.Command.ZINCRBY, 3);
+ }
+
+ @Test
+ public void shouldError_givenNonFloatIncrement() {
+ String nonFloatIncrement = "q";
+ jedis.zadd(KEY, 1.0, MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nonFloatIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ @Test
+ public void shouldError_givenNaNIncrement() {
+ String nanIncrement = "NaN";
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+
+ assertThatThrownBy(() -> jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY,
+ nanIncrement, STRING_MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+
+ /************* infinity *************/
+ @Test
+ public void shouldSetScoreToInfinity_ifIncrementWouldExceedMaxValue() {
+ double increment = Double.MAX_VALUE / 2;
+
+ jedis.zadd(STRING_KEY, Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, -Double.MAX_VALUE, STRING_MEMBER);
+ jedis.zincrby(STRING_KEY, -increment, STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void shouldSetScoreToInfinity_givenInfiniteIncrement() {
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, POSITIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ assertThat(jedis.zincrby(STRING_KEY, NEGATIVE_INFINITY, STRING_MEMBER))
+ .isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-inf", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "+Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zadd(STRING_KEY, 1.0, STRING_MEMBER);
+ jedis.sendCommand(STRING_KEY, Protocol.Command.ZINCRBY, STRING_KEY, "-Infinity", STRING_MEMBER);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
+
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void nullRedisSetShouldCreateNonExistentMember_withIncrementOfNegativeInfinity() {
+ final double increment = NEGATIVE_INFINITY;
+ jedis.zadd(KEY, 0.0, "something".getBytes()); // init the key but not the member
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void shouldCreateNonExistentKeyWithInfiniteScore_whenIncrementIsPositiveInfinity() {
+ final double increment = POSITIVE_INFINITY;
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void shouldCreateNonExistentKeyWithInfiniteScore_whenIncrementIsNegativeInfinity() {
+ final double increment = NEGATIVE_INFINITY;
+ jedis.zincrby(KEY, increment, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void scoreOfPositiveInfinityShouldRemainInfinite_whenIncrementingOrDecrementing() {
+ jedis.zadd(KEY, POSITIVE_INFINITY, MEMBER);
+
+ jedis.zincrby(KEY, 100.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(POSITIVE_INFINITY);
+
+ jedis.zincrby(KEY, -1000.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(POSITIVE_INFINITY);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void scoreOfNegativeInfinityShouldRemainInfinite_whenIncrementingOrDecrementing() {
+ jedis.zadd(KEY, NEGATIVE_INFINITY, MEMBER);
+
+ jedis.zincrby(KEY, 100.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+
+ jedis.zincrby(KEY, -1000.0, MEMBER);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(NEGATIVE_INFINITY);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void incrementingScoreOfInfinityByNegativeInfinity_throwsNaNError() {
+ jedis.zadd(KEY, POSITIVE_INFINITY, MEMBER);
+
+ assertThatThrownBy(() -> jedis.zincrby(KEY, NEGATIVE_INFINITY, MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_OPERATION_PRODUCED_NAN);
+ }
+
+ @Ignore("blocked by GEODE-9317")
+ @Test
+ public void incrementingScoreOfNegativeInfinityByPositiveInfinity_throwsNaNError() {
+ jedis.zadd(KEY, NEGATIVE_INFINITY, MEMBER);
+
+ assertThatThrownBy(() -> jedis.zincrby(KEY, POSITIVE_INFINITY, MEMBER))
+ .hasMessageContaining(RedisConstants.ERROR_OPERATION_PRODUCED_NAN);
+ }
+
+ /************* key or member does not exist *************/
+ @Test
+ public void shouldCreateNewKey_whenIncrementedKeyDoesNotExist() {
+ final double increment = 1.5;
+
+ assertThat(jedis.zincrby(KEY, increment, MEMBER)).isEqualTo(increment);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ @Test
+ public void shouldCreateNewMember_whenIncrementedMemberDoesNotExist() {
+ final double increment = 1.5;
+ jedis.zadd(KEY, increment, "something".getBytes());
+
+ assertThat(jedis.zincrby(KEY, increment, MEMBER)).isEqualTo(increment);
+ assertThat(jedis.zscore(KEY, MEMBER)).isEqualTo(increment);
+ }
+
+ /************* happy path *************/
+ @Test
+ public void shouldIncrementScore_givenKeyAndMemberExist() {
+ testZIncrByIncrements(KEY, MEMBER, 2.7, 1.5);
+ }
+
+ @Test
+ public void shouldIncrementScore_givenNegativeIncrement() {
+ testZIncrByIncrements(KEY, MEMBER, 2.7, -1.5);
+ }
+
+ @Test
+ public void shouldIncrementScoreToANegativeNumber_givenNegativeIncrementGreaterThanScore() {
+ testZIncrByIncrements(KEY, MEMBER, 2.7, -5.6);
+ }
+
+ @Test
+ public void shouldIncrementScore_givenNegativeInitialScoreAndPositiveIncrement() {
+ testZIncrByIncrements(KEY, MEMBER, -2.7, 1.7);
+ }
+
+ @Test
+ public void shouldIncrementScore_givenIncrementInExponentialForm() {
+ testZIncrByIncrements(KEY, MEMBER, -2.7, 2e5);
+ testZIncrByIncrements(KEY, MEMBER, 2e5, -2.7);
+ }
+
+ /************* concurrency *************/
+ @Test
+ public void testConcurrentZIncrBy_performsAllZIncrBys() {
+ AtomicDouble expectedValue = new AtomicDouble(0.0);
+
+ jedis.zadd(STRING_KEY, expectedValue.get(), STRING_MEMBER);
+
+ new ConcurrentLoopingThreads(1000,
+ (i) -> {
+ double increment = ThreadLocalRandom.current().nextDouble(-50, 50);
+ expectedValue.addAndGet(increment);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ },
+ (i) -> {
+ double increment = ThreadLocalRandom.current().nextDouble(-50, 50);
+ expectedValue.addAndGet(increment);
+ jedis.zincrby(STRING_KEY, increment, STRING_MEMBER);
+ }).run();
+
+ Offset<Double> offset = Offset.offset(Math.abs(expectedValue.get()) * 0.05);
+ assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isCloseTo(expectedValue.get(), offset);
Review comment:
Is there a reason that this can't just be
```
assertThat(jedis.zscore(STRING_KEY, STRING_MEMBER)).isEqualTo(expectedValue.get());
```
Given the test name, if the score is only close to the expected value rather than exactly equal, we didn't perform all the `zincrby`s and so the test should fail.
--
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