DonalEvans commented on a change in pull request #6783:
URL: https://github.com/apache/geode/pull/6783#discussion_r702155507
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -213,34 +232,42 @@ 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);
+ }
+
+ if (membersToAdd.size() != scoresToAdd.size()) {
+ throw new NumberFormatException("there must be the same number of
members and scores");
}
- AddsDeltaInfo deltaInfo = null;
- Iterator<byte[]> iterator = membersToAdd.iterator();
+
+ ZaddsDeltaInfo deltaInfo = null;
+ Iterator<byte[]> membersIterator = membersToAdd.iterator();
+ Iterator<Double> scoresIterator = scoresToAdd.iterator();
+
int initialSize = scoreSet.size();
int changesCount = 0;
- while (iterator.hasNext()) {
- byte[] score = iterator.next();
- byte[] member = iterator.next();
+
+ while (membersIterator.hasNext()) {
+ double score = scoresIterator.next();
+ byte[] member = membersIterator.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 = new ZaddsDeltaInfo(member, score);
+ } else {
+ deltaInfo.add(member, score);
}
Review comment:
I realize now that this suggestion is incorrect, because we actually
have three possible cases: a new entry was added, an existing entry was updated
or an existing entry was not updated. We need to know if the second case is
true when the `CH` option is selected, but if the third case is true, then we
don't need to send a delta or update the region. It would be good if we could
find a way to cover both cases, since updating the region for no-ops seems
wasteful.
--
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]
