dschneider-pivotal commented on a change in pull request #6794:
URL: https://github.com/apache/geode/pull/6794#discussion_r700570207
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -303,16 +316,25 @@ public RedisKeyCommands getKeyCommands() {
*
* @return the keys ordered in the sequence in which they should be locked.
*/
- public List<RedisKey> orderForLocking(RedisKey key1, RedisKey key2) {
- List<RedisKey> orderedKeys = new ArrayList<>();
- if (stripedCoordinator.compareStripes(key1, key2) > 0) {
- orderedKeys.add(key1);
- orderedKeys.add(key2);
- } else {
- orderedKeys.add(key2);
- orderedKeys.add(key1);
- }
+ public List<RedisKey> orderForLocking(List<RedisKey> keys) {
Review comment:
since this does an inplace sort of the keys param, would something like
this make that more clear: "public void sortForLocking(List<RedisKey> keys)"
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -383,6 +389,51 @@ long zrevrank(byte[] member) {
return null;
}
+ long zunionstore(RegionProvider regionProvider, RedisKey key,
List<ZKeyWeight> keyWeights,
+ ZAggregator aggregator) {
+ for (ZKeyWeight keyWeight : keyWeights) {
+ RedisSortedSet set =
+ regionProvider.getTypedRedisData(REDIS_SORTED_SET,
keyWeight.getKey(), false);
+ if (set == NULL_REDIS_SORTED_SET) {
+ continue;
+ }
+ double weight = keyWeight.getWeight();
+
+ for (AbstractOrderedSetEntry entry : set.members.values()) {
+ OrderedSetEntry existingValue = members.get(entry.member);
+ if (existingValue == null) {
+ byte[] score;
Review comment:
name this "scoreBytes"
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java
##########
@@ -126,4 +130,19 @@ public long zrevrank(RedisKey key, byte[] member) {
public byte[] zscore(RedisKey key, byte[] member) {
return stripedExecute(key, () -> getRedisSortedSet(key,
true).zscore(member));
}
+
+ @Override
+ public long zunionstore(RedisKey destinationKey, List<ZKeyWeight> keyWeights,
+ ZAggregator aggregator) {
+ List<RedisKey> keysToLock = keyWeights.stream()
+ .collect(ArrayList::new, (x, y) -> x.add(y.getKey()),
ArrayList::addAll);
+ keysToLock.add(destinationKey);
+ return stripedExecute(destinationKey, keysToLock,
+ () -> {
+ getRegionProvider().ensureKeyIsLocal(destinationKey);
Review comment:
consider moving this ensureKeyIsLocal check down into
RedisSortedSet.zunionstore or do it at the beginning of this method. Does it
need to be done from the stripedExecute?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java
##########
@@ -126,4 +130,19 @@ public long zrevrank(RedisKey key, byte[] member) {
public byte[] zscore(RedisKey key, byte[] member) {
return stripedExecute(key, () -> getRedisSortedSet(key,
true).zscore(member));
}
+
+ @Override
+ public long zunionstore(RedisKey destinationKey, List<ZKeyWeight> keyWeights,
+ ZAggregator aggregator) {
+ List<RedisKey> keysToLock = keyWeights.stream()
+ .collect(ArrayList::new, (x, y) -> x.add(y.getKey()),
ArrayList::addAll);
Review comment:
this stream code is hard for me to understand. Could you convert it to a
for loop?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -383,6 +389,51 @@ long zrevrank(byte[] member) {
return null;
}
+ long zunionstore(RegionProvider regionProvider, RedisKey key,
List<ZKeyWeight> keyWeights,
+ ZAggregator aggregator) {
+ for (ZKeyWeight keyWeight : keyWeights) {
+ RedisSortedSet set =
+ regionProvider.getTypedRedisData(REDIS_SORTED_SET,
keyWeight.getKey(), false);
+ if (set == NULL_REDIS_SORTED_SET) {
+ continue;
+ }
+ double weight = keyWeight.getWeight();
+
+ for (AbstractOrderedSetEntry entry : set.members.values()) {
+ OrderedSetEntry existingValue = members.get(entry.member);
+ if (existingValue == null) {
+ byte[] score;
+ // Redis math and Java math are different when handling infinity.
Specifically:
+ // Java: INFINITY * 0 = NaN
+ // Redis: INFINITY * 0 = 0
+ if (weight == 0) {
+ score = bZERO;
+ } else if (weight == 1) {
+ score = entry.getScoreBytes();
+ } else {
+ double newScore = entry.score * weight;
+ if (Double.isNaN(newScore)) {
+ score = entry.getScoreBytes();
+ } else {
+ score = Coder.doubleToBytes(entry.score * weight);
+ }
+ }
+ members.put(entry.member, new OrderedSetEntry(entry.member, score));
+ continue;
+ }
+
+
existingValue.updateScore(aggregator.getFunction().apply(existingValue.score,
+ entry.score * weight));
Review comment:
you had a comment up above about how redis and java do math differently.
Why can this code just do java math?
--
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]
