dschneider-pivotal commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r695936459
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -274,9 +277,9 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
+ memberAdd(member, incr);
+ byte[] byteIncr = doubleToBytes(incr);
Review comment:
use the new ZAddsDeltaInfo here to send "incr"
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -274,9 +277,9 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
}
}
- byte[] byteIncr = doubleToBytes(incr);
- memberAdd(member, byteIncr);
+ memberAdd(member, incr);
Review comment:
This method is a bit confusing because it increments "incr" instead of
incrementing "byteScore" by "incr". I think it would be easier to understand if
"incr" always represented increment. I know this pr did not make this choice
but if you end up working on this method more it would be nice to clean that
up. It confused me at first because when I saw it adding "byteIncr" to the
deltaInfo I thought it was distributing the increment but it is actually the
new incremented value.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -221,25 +225,24 @@ Object zadd(Region<RedisKey, RedisData> region, RedisKey
key, List<byte[]> membe
int initialSize = scoreSet.size();
int changesCount = 0;
while (iterator.hasNext()) {
- byte[] score = iterator.next();
+ double score = Coder.bytesToDouble(iterator.next());
byte[] member = iterator.next();
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.add(member);
- deltaInfo.add(score);
+ deltaInfo.add(Coder.doubleToBytes(score));
Review comment:
don't call doubleToBytes here. You have already done the work of
converting the score to a double and by shipping the bytes in the DeltaInfo
that work is repeated when the delta is applied on the other side. I think you
should just add a new flavor of DeltaInfo. You could name it ZAddsDeltaInfo and
its add signature would be "add(byte[], double)". It does not matter that much
how you store the data in the DeltaInfo but when you serialize it just write
the item count (using InternalDataSerializer.writeArrayLength) and then write a
byte[] (using DataSerializer.writeByteArray) and a double (using
DataSerializer.writePrimitiveDouble). Also this current code does an odd thing
when constructing AddsDeltaInfo. It uses a constructor that takes an ArrayList
but that contructor is really meant for copying a list into the AddsDeltaInfo.
Instead it should have used the AddsDeltaInfo(size) constructor and used the
size of membersToAdd to as the parameter. You will want to do this for your
new ZAddsDeltaInfo
--
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]
