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 2022/01/04 23:11:09 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7219: feature/GEODE-9902: Modify ZUNIONSTORE and ZINTERSTORE storing methods
DonalEvans commented on a change in pull request #7219:
URL: https://github.com/apache/geode/pull/7219#discussion_r778402027
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -323,18 +339,48 @@ public long zinterstore(RegionProvider regionProvider, RedisKey key, List<ZKeyWe
}
}
}
+
if (addToSet) {
- memberAdd(member, newScore);
+ OrderedSetEntry entry = interMembers.get(member);
+ if (entry == null) {
+ entry = new OrderedSetEntry(member, newScore);
+ interMembers.put(member, entry);
+ interScores.add(entry);
+ } else if (entry.getScore() != newScore) {
+ interScores.remove(entry);
+ entry.updateScore(newScore);
+ interScores.add(entry);
+ }
}
}
- if (removeFromRegion()) {
- regionProvider.getDataRegion().remove(key);
- } else {
- regionProvider.getLocalDataRegion().put(key, this);
+ return sortedSetOpStoreResult(regionProvider, key, interMembers, interScores);
+ }
+
+ @VisibleForTesting
+ static long sortedSetOpStoreResult(RegionProvider regionProvider, RedisKey destinationKey,
+ MemberMap interMembers, ScoreSet interScores) {
+ RedisSortedSet destinationSet =
+ regionProvider.getTypedRedisDataElseRemove(REDIS_SORTED_SET, destinationKey, false);
Review comment:
The `getTypedRedisDataElseRemove()` method has a `TODO` on it saying it should be removed once SDIFFSTORE is merged. With its use here, I assume that is no longer correct. Could the comment be removed?
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -323,18 +339,48 @@ public long zinterstore(RegionProvider regionProvider, RedisKey key, List<ZKeyWe
}
}
}
+
if (addToSet) {
- memberAdd(member, newScore);
+ OrderedSetEntry entry = interMembers.get(member);
+ if (entry == null) {
+ entry = new OrderedSetEntry(member, newScore);
+ interMembers.put(member, entry);
+ interScores.add(entry);
+ } else if (entry.getScore() != newScore) {
+ interScores.remove(entry);
+ entry.updateScore(newScore);
+ interScores.add(entry);
+ }
}
}
- if (removeFromRegion()) {
- regionProvider.getDataRegion().remove(key);
- } else {
- regionProvider.getLocalDataRegion().put(key, this);
+ return sortedSetOpStoreResult(regionProvider, key, interMembers, interScores);
+ }
+
+ @VisibleForTesting
+ static long sortedSetOpStoreResult(RegionProvider regionProvider, RedisKey destinationKey,
+ MemberMap interMembers, ScoreSet interScores) {
+ RedisSortedSet destinationSet =
+ regionProvider.getTypedRedisDataElseRemove(REDIS_SORTED_SET, destinationKey, false);
+
+ if (interMembers == null || interScores.isEmpty()) {
Review comment:
The LGTM warning here can be fixed either by making this:
```
if (interMembers == null || interScores == null || interScores.isEmpty()) {
```
or by changing it to:
```
if (interMembers.isEmpty() || interScores.isEmpty()) {
```
and changing the call to `sortedSetOpStoreResult()` on line 299 to pass an empty `MemberMap` and empty `ScoreSet` rather than `null`. I would tend to prefer the second approach, as avoiding passing null around is generally better.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -463,18 +509,24 @@ public long zrevrank(byte[] member) {
return null;
}
- public long zunionstore(RegionProvider regionProvider, RedisKey key, List<ZKeyWeight> keyWeights,
+ public static long zunionstore(RegionProvider regionProvider, RedisKey key,
+ List<ZKeyWeight> keyWeights,
ZAggregator aggregator) {
+ MemberMap unionMembers = null;
+ ScoreSet unionScores = null;
for (ZKeyWeight keyWeight : keyWeights) {
RedisSortedSet set =
regionProvider.getTypedRedisData(REDIS_SORTED_SET, keyWeight.getKey(), false);
if (set == NULL_REDIS_SORTED_SET) {
continue;
+ } else if (unionMembers == null) {
+ unionMembers = new MemberMap((int) set.zcard());
+ unionScores = new ScoreSet();
}
double weight = keyWeight.getWeight();
for (AbstractOrderedSetEntry entry : set.members.values()) {
Review comment:
Minor thing, but the code in this for loop could be made a little more readable by changing the if to and if/else, removing the `continue` and having the if condition be `if (existingValue != null)`. This puts the smaller block of code at the top of the if statement, which improves readability, and makes it clear that there are two possible cases with two different behaviours rather than an if followed by other code, which could imply that the code outside the if is expected to execute regardless of how the if condition evaluates.
##########
File path: geode-for-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSortedSetTest.java
##########
@@ -153,6 +157,58 @@ public void zadd_stores_delta_that_is_stable() {
assertThat(sortedSet1.hasDelta()).isFalse();
}
+ @Test
+ public void zstoreoperations_stores_delta_that_is_stable() {
Review comment:
This test and the one below it should probably be renamed "sortedSetOpStoreResult_*" as there is no method named "zstoreoperations".
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -463,18 +509,24 @@ public long zrevrank(byte[] member) {
return null;
}
- public long zunionstore(RegionProvider regionProvider, RedisKey key, List<ZKeyWeight> keyWeights,
+ public static long zunionstore(RegionProvider regionProvider, RedisKey key,
+ List<ZKeyWeight> keyWeights,
ZAggregator aggregator) {
+ MemberMap unionMembers = null;
+ ScoreSet unionScores = null;
Review comment:
Rather than declaring these as null and only assigning them if we find a set, I think it might be better to create them up front and avoid them ever being null when passed into `sortedSetOpStoreResult()`. While this doesn't allow an initial size to be set for `unionMembers`, that's not much of a loss of optimization, since even if we do use the first set we find to set an initial size, it's entirely possible that the initial size is far smaller than the final size and so we end up doing a bunch of resizes anyway.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -323,18 +339,48 @@ public long zinterstore(RegionProvider regionProvider, RedisKey key, List<ZKeyWe
}
}
}
+
if (addToSet) {
- memberAdd(member, newScore);
+ OrderedSetEntry entry = interMembers.get(member);
+ if (entry == null) {
Review comment:
This check will always return true, because we are iterating the contents of `smallestSet` (which we know doesn't contain any duplicate entries) and only adding to `interMembers` in one place, so it's not possible that it will already contain the member.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org