DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r705648542
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSortedSet.java
##########
@@ -64,16 +65,18 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
@Override
- byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, byte[]
increment,
+ byte[] zincrby(Region<RedisKey, RedisData> region, RedisKey key, double
increment,
byte[] member) {
- List<byte[]> valuesToAdd = new ArrayList<>();
- valuesToAdd.add(increment);
- valuesToAdd.add(member);
+ List<byte[]> membersToAdd = new ArrayList<>();
+ membersToAdd.add(member);
- RedisSortedSet sortedSet = new RedisSortedSet(valuesToAdd);
+ List<Double> scoresToAdd = new ArrayList<>();
+ scoresToAdd.add(increment);
+
+ RedisSortedSet sortedSet = new RedisSortedSet(membersToAdd, scoresToAdd);
Review comment:
This can be simplified to
```
RedisSortedSet sortedSet = new RedisSortedSet(singletonList(member),
singletonList(increment));
```
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/collections/SizeableBytes2ObjectOpenCustomHashMapWithCursorTest.java
##########
@@ -364,24 +364,22 @@ public void
getSizeInBytesIsAccurateForOrderedSetEntryValues() {
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
byte[] member = key;
- byte[] scoreBytes = String.valueOf(totalNumberOfElements - i).getBytes();
- RedisSortedSet.OrderedSetEntry value = new
RedisSortedSet.OrderedSetEntry(member, scoreBytes);
+ double score = totalNumberOfElements - i;
+ RedisSortedSet.OrderedSetEntry value = new
RedisSortedSet.OrderedSetEntry(member, score);
hash.put(key, value);
assertThat(hash.getSizeInBytes()).isEqualTo(sizer.sizeof(hash));
}
// Update values and confirm that size changes as expected
for (int i = initialNumberOfElements; i < totalNumberOfElements; ++i) {
byte[] key = {(byte) i};
- byte[] member = key;
- byte[] scoreBytes = String.valueOf(i).getBytes();
RedisSortedSet.OrderedSetEntry value = hash.get(key);
- byte[] oldScoreBytes = value.getScoreBytes();
- int scoreDelta = memoryOverhead(scoreBytes)
- - memoryOverhead(oldScoreBytes);
+ double oldScore = value.getScore();
+ int scoreDelta =
+ memoryOverhead(Coder.doubleToBytes(i)) -
memoryOverhead(Coder.doubleToBytes(oldScore));
int oldSize = hash.getSizeInBytes();
- value.updateScore(scoreBytes);
+ value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isEqualTo(scoreDelta);
Review comment:
The scoreDelta here should be 0, since `double` is fixed size and
updating its value does not change its size. This section can then be
refactored to:
```
RedisSortedSet.OrderedSetEntry value = hash.get(key);
int oldSize = hash.getSizeInBytes();
value.updateScore(i);
int sizeDelta = hash.getSizeInBytes() - oldSize;
assertThat(sizeDelta).isZero();
```
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -50,8 +53,17 @@ public RedisResponse executeCommand(Command command,
ExecutionHandlerContext con
return RedisResponse.error(zAddExecutorState.exceptionMessage);
}
- Object retVal = redisSortedSetCommands.zadd(command.getKey(),
- new ArrayList<>(commandElements.subList(optionsFoundCount + 2,
commandElements.size())),
+ List<byte[]> members = new ArrayList<>();
+ List<Double> scores = new ArrayList<>();
Review comment:
These lists can be created with appropriate sizes using something like:
```
int size = (commandElements.size() - optionsFoundCount - 2) / 2;
List<byte[]> members = new ArrayList<>(size);
List<Double> scores = new ArrayList<>(size);
```
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/netty/CoderTest.java
##########
@@ -105,16 +110,6 @@ public void narrowLongToInt_correctlyConvertsLongToInt() {
}
}
- @Test
- @Parameters(method = "doubleBytes")
- public void stripTrailingZeroFromDouble_correctlyStripsTrailingZero(byte[]
input,
Review comment:
With the removal of this test, it would be good to add an equivalent one
that tests the behaviour in `Coder.doubleToBytes()` that strips trailing zeroes
from double values. The currently unused `doubleBytes()` method in this test
class could be fairly easily repurposed to facilitate this:
```
@Test
@Parameters(method = "doubleBytes")
public void doubleToBytes_processesTrailingZeroLikeRedis(Double
inputDouble, byte[] expectedBytes) {
double d = inputDouble;
byte[] convertedBytes = doubleToBytes(d);
assertArrayEquals(convertedBytes, expectedBytes);
}
@SuppressWarnings("unused")
private Object[] doubleBytes() {
// input double, double bytes with stripped trailing zero
return new Object[] {
new Object[] {0.0, stringToBytes("0")},
new Object[] {0.01, stringToBytes("0.01")},
new Object[] {0d, stringToBytes("0")},
new Object[] {1.0, stringToBytes("1")},
new Object[] {-1.0, stringToBytes("-1")},
new Object[] {6.0221409E23, stringToBytes("6.0221409E23")},
new Object[] {6.62607E-34, stringToBytes("6.62607E-34")},
new Object[] {Double.POSITIVE_INFINITY, stringToBytes("inf")},
new Object[] {Double.NaN, stringToBytes("NaN")},
};
}
```
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -210,34 +206,34 @@ private synchronized void membersRemoveAll(RemsDeltaInfo
remsDeltaInfo) {
* @return the number of members actually added OR incremented value if INCR
option specified
*/
Object zadd(Region<RedisKey, RedisData> region, RedisKey key, List<byte[]>
membersToAdd,
- ZAddOptions options) {
+ List<Double> scoresToAdd, ZAddOptions options) {
if (options.isINCR()) {
- return zaddIncr(region, key, membersToAdd, options);
+ // if INCR, only one score and member can be added
+ return zaddIncr(region, key, membersToAdd.get(0), scoresToAdd.get(0),
options);
}
- AddsDeltaInfo deltaInfo = null;
- Iterator<byte[]> iterator = membersToAdd.iterator();
+
+ ZAddsDeltaInfo deltaInfo = null;
int initialSize = scoreSet.size();
int changesCount = 0;
- while (iterator.hasNext()) {
- byte[] score = iterator.next();
- byte[] member = iterator.next();
+
+ for (int i = 0; i < membersToAdd.size(); i++) {
+ double score = scoresToAdd.get(i);
+ byte[] member = membersToAdd.get(i);
if (options.isNX() && members.containsKey(member)) {
continue;
}
if (options.isXX() && !members.containsKey(member)) {
continue;
}
- byte[] oldScore = memberAdd(member, score);
- if (options.isCH() && oldScore != null
- && !Arrays.equals(oldScore, stripTrailingZeroFromDouble(score))) {
+ boolean scoreChanged = memberAdd(member, score);
+ if (options.isCH() && scoreChanged) {
changesCount++;
}
-
if (deltaInfo == null) {
- deltaInfo = new AddsDeltaInfo(new ArrayList<>());
+ deltaInfo = new ZAddsDeltaInfo(member, score);
+ } else {
+ deltaInfo.add(member, score);
}
- deltaInfo.add(member);
- deltaInfo.add(score);
}
Review comment:
I had a bit more of a think about the behaviour here and have a
potential solution to us sending Deltas and updating the region for no-op
updates (where the score is not changed). I added an enum to the class:
```
private enum MemberAddResult {
CREATE,
UPDATE,
NO_OP
}
```
Then I modified the `memberAdd()` method:
```
protected synchronized MemberAddResult memberAdd(byte[] memberToAdd,
double scoreToAdd) {
OrderedSetEntry entry = members.get(memberToAdd);
if (entry == null) {
OrderedSetEntry newEntry = new OrderedSetEntry(memberToAdd,
scoreToAdd);
members.put(memberToAdd, newEntry);
scoreSet.add(newEntry);
return MemberAddResult.CREATE;
} else {
if (entry.score == scoreToAdd) {
return MemberAddResult.NO_OP;
}
scoreSet.remove(entry);
entry.updateScore(scoreToAdd);
scoreSet.add(entry);
return MemberAddResult.UPDATE;
}
}
```
and finally updated the code here to make use of the new enum:
```
MemberAddResult addResult = memberAdd(member, score);
if (options.isCH() && addResult.equals(MemberAddResult.UPDATE)) {
changesCount++;
}
if (!addResult.equals(MemberAddResult.NO_OP)) {
if (deltaInfo == null) {
deltaInfo = new ZAddsDeltaInfo(member, score);
} else {
deltaInfo.add(member, score);
}
}
```
This is possibly overkill, but it does prevent us sending deltas when we
don't need to, which is a small performance optimization. The approach used
here (only creating/adding to `ZAddsDeltaInfo` if the result is not `NO_OP`)
can also be used everywhere else `memberAdd()` is called to reduce the number
of unnecessary deltas we're sending.
##########
File path:
geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
##########
@@ -147,11 +147,13 @@ public void zadd_stores_delta_that_is_stable() {
Region<RedisKey, RedisData> region = uncheckedCast(mock(Region.class));
when(region.put(any(),
any())).thenAnswer(this::validateDeltaSerialization);
RedisSortedSet sortedSet1 = createRedisSortedSet("3.14159", "v1",
"2.71828", "v2");
- List<byte[]> adds = new ArrayList<>();
- adds.add(stringToBytes("1.61803"));
- adds.add(stringToBytes("v3"));
+ List<byte[]> members = new ArrayList<>();
+ members.add(stringToBytes("v3"));
+ List<Double> scores = new ArrayList<>();
+ scores.add(1.61803);
Review comment:
This can be simplified to:
```
List<byte[]> members = singletonList(stringToBytes("v3"));
List<Double> scores = singletonList(1.61803);
```
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/Coder.java
##########
@@ -411,9 +411,31 @@ public static double bytesToDouble(byte[] bytes) {
return NEGATIVE_INFINITY;
}
if (isNaN(bytes)) {
- return NaN;
+ throw new NumberFormatException();
+ }
+
+ try {
+ return stringToDouble(bytesToString(bytes));
+ } catch (NumberFormatException e) {
+ throw new NumberFormatException(RedisConstants.ERROR_NOT_A_VALID_FLOAT);
+ }
+ }
+
+ /**
+ * Redis specific manner to parse floats
+ *
+ * @param d String holding double
+ * @return Value of string
+ * @throws NumberFormatException if the double cannot be parsed
+ */
+ public static double stringToDouble(String d) {
+ if (d.equalsIgnoreCase(P_INF)) {
+ return Double.POSITIVE_INFINITY;
+ } else if (d.equalsIgnoreCase(N_INF)) {
Review comment:
This method is only called from within `Coder` (and a test class which
uses it as a convenience method), where it's preceded with checks for other
formats of infinity. It might be best to just make this method private or
inline it into the `bytesToDouble()` method, since internally we should never
need to handle the String representation of a double.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java
##########
@@ -89,6 +101,9 @@ private int findAndValidateZAddOptions(Command command,
Iterator<byte[]> command
optionsFoundCount++;
} else if (isInfinity(subCommand)) {
scoreFound = true;
+ } else if (isNaN(subCommand)) {
+ executorState.exceptionMessage = ERROR_NOT_A_VALID_FLOAT;
+ return 0;
Review comment:
> With the call to `Coder.bytesToDouble()` on line 60, this check
shouldn't be needed, as the modified `bytesToDouble()` method will now throw an
exception with the correct message when we get to that line. We would do a
couple of unnecessary allocations if a user has passed NaN as a score with this
removed, but we would avoid doing a byte array comparison on every argument in
the more likely case where they haven't passed NaN, so it's a net performance
improvement, I think.
This comment is still relevant, I think.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
