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/03 18:50:36 UTC
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7228: feature/GEODE-9834: SRANDMEMBER Command Support
dschneider-pivotal commented on a change in pull request #7228:
URL: https://github.com/apache/geode/pull/7228#discussion_r777630052
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -161,49 +162,42 @@ public RedisSet() {}
return popped;
}
- public Collection<byte[]> srandmember(int count) {
- int membersSize = members.size();
+ public List<byte[]> srandmember(int count) {
boolean duplicatesAllowed = count < 0;
- if (duplicatesAllowed) {
- count = -count;
- }
-
- if (!duplicatesAllowed && membersSize <= count && count != 1) {
- return new ArrayList<>(members);
- }
+ count = duplicatesAllowed ? -count : count;
+ int membersSize = members.size();
+ int memberMapSize = members.getMemberMapSize();
Random rand = new Random();
-
- // TODO: this could be optimized to take advantage of MemberSet
- // storing its data in an array. We probably don't need to copy it
- // into another array here.
- byte[][] entries = members.toArray(new byte[membersSize][]);
-
- if (count == 1) {
- byte[] randEntry = entries[rand.nextInt(entries.length)];
- // TODO: Now that the result is no longer serialized this could use singleton.
- // Note using ArrayList because Collections.singleton has serialization issues.
- List<byte[]> result = new ArrayList<>(1);
- result.add(randEntry);
- return result;
- }
- if (duplicatesAllowed) {
- List<byte[]> result = new ArrayList<>(count);
- while (count > 0) {
- result.add(entries[rand.nextInt(entries.length)]);
- count--;
- }
- return result;
+ List<byte[]> randMembers = new ArrayList<>(count);
+ if (!duplicatesAllowed && membersSize <= count) {
+ randMembers.addAll(members);
} else {
- Set<byte[]> result = new MemberSet(count);
- // Note that rand.nextInt can return duplicates when "count" is high
- // so we need to use a Set to collect the results.
- while (result.size() < count) {
- byte[] s = entries[rand.nextInt(entries.length)];
- result.add(s);
+ Set<Integer> usedPos = null;
+ if (!duplicatesAllowed) {
+ usedPos = new HashSet<>(count);
+ }
+
+ for (int i = 0; i < count; i++) {
+ int randNum = rand.nextInt(memberMapSize);
Review comment:
would "randomIndex" be a better name?
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SRandMemberExecutor.java
##########
@@ -14,55 +14,13 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
-import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
-import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
-
-import java.util.Collection;
import java.util.List;
-import org.apache.geode.redis.internal.commands.Command;
-import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
-import org.apache.geode.redis.internal.commands.executor.RedisResponse;
-import org.apache.geode.redis.internal.data.RedisKey;
-import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
-
-public class SRandMemberExecutor implements CommandExecutor {
-
- private static final String ERROR_NOT_NUMERIC = "The count provided must be numeric";
+import org.apache.geode.redis.internal.data.RedisSet;
+public class SRandMemberExecutor extends SetRandomExecutor {
@Override
- public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
-
- RedisKey key = command.getKey();
-
- boolean countSpecified = false;
- int count;
-
- if (commandElems.size() > 2) {
- try {
- count = narrowLongToInt(bytesToLong(commandElems.get(2)));
- countSpecified = true;
- } catch (NumberFormatException e) {
- return RedisResponse.error(ERROR_NOT_NUMERIC);
- }
- } else {
- count = 1;
- }
-
- if (count == 0) {
- return RedisResponse.emptyArray();
- }
-
- Collection<byte[]> results = context.setLockedExecute(key, true,
- set -> set.srandmember(count));
-
- if (countSpecified) {
- return RedisResponse.array(results, true);
- } else if (results.isEmpty()) {
- return RedisResponse.nil();
- } else {
- return RedisResponse.bulkString(results.iterator().next());
- }
+ protected List<byte[]> preformCommand(RedisSet set, int count) {
Review comment:
is the name a typo? I think it should be "perform" instead of "preform"
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -161,49 +162,42 @@ public RedisSet() {}
return popped;
}
- public Collection<byte[]> srandmember(int count) {
- int membersSize = members.size();
+ public List<byte[]> srandmember(int count) {
boolean duplicatesAllowed = count < 0;
- if (duplicatesAllowed) {
- count = -count;
- }
-
- if (!duplicatesAllowed && membersSize <= count && count != 1) {
- return new ArrayList<>(members);
- }
+ count = duplicatesAllowed ? -count : count;
+ int membersSize = members.size();
+ int memberMapSize = members.getMemberMapSize();
Random rand = new Random();
-
- // TODO: this could be optimized to take advantage of MemberSet
- // storing its data in an array. We probably don't need to copy it
- // into another array here.
- byte[][] entries = members.toArray(new byte[membersSize][]);
-
- if (count == 1) {
- byte[] randEntry = entries[rand.nextInt(entries.length)];
- // TODO: Now that the result is no longer serialized this could use singleton.
- // Note using ArrayList because Collections.singleton has serialization issues.
- List<byte[]> result = new ArrayList<>(1);
- result.add(randEntry);
- return result;
- }
- if (duplicatesAllowed) {
- List<byte[]> result = new ArrayList<>(count);
- while (count > 0) {
- result.add(entries[rand.nextInt(entries.length)]);
- count--;
- }
- return result;
+ List<byte[]> randMembers = new ArrayList<>(count);
+ if (!duplicatesAllowed && membersSize <= count) {
+ randMembers.addAll(members);
} else {
- Set<byte[]> result = new MemberSet(count);
- // Note that rand.nextInt can return duplicates when "count" is high
- // so we need to use a Set to collect the results.
- while (result.size() < count) {
- byte[] s = entries[rand.nextInt(entries.length)];
- result.add(s);
+ Set<Integer> usedPos = null;
Review comment:
would "usedIndexes" be a better name?
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -161,49 +162,42 @@ public RedisSet() {}
return popped;
}
- public Collection<byte[]> srandmember(int count) {
- int membersSize = members.size();
+ public List<byte[]> srandmember(int count) {
boolean duplicatesAllowed = count < 0;
- if (duplicatesAllowed) {
- count = -count;
- }
-
- if (!duplicatesAllowed && membersSize <= count && count != 1) {
- return new ArrayList<>(members);
- }
+ count = duplicatesAllowed ? -count : count;
+ int membersSize = members.size();
+ int memberMapSize = members.getMemberMapSize();
Random rand = new Random();
-
- // TODO: this could be optimized to take advantage of MemberSet
- // storing its data in an array. We probably don't need to copy it
- // into another array here.
- byte[][] entries = members.toArray(new byte[membersSize][]);
-
- if (count == 1) {
- byte[] randEntry = entries[rand.nextInt(entries.length)];
- // TODO: Now that the result is no longer serialized this could use singleton.
- // Note using ArrayList because Collections.singleton has serialization issues.
- List<byte[]> result = new ArrayList<>(1);
- result.add(randEntry);
- return result;
- }
- if (duplicatesAllowed) {
- List<byte[]> result = new ArrayList<>(count);
- while (count > 0) {
- result.add(entries[rand.nextInt(entries.length)]);
- count--;
- }
- return result;
+ List<byte[]> randMembers = new ArrayList<>(count);
+ if (!duplicatesAllowed && membersSize <= count) {
+ randMembers.addAll(members);
} else {
- Set<byte[]> result = new MemberSet(count);
- // Note that rand.nextInt can return duplicates when "count" is high
- // so we need to use a Set to collect the results.
- while (result.size() < count) {
- byte[] s = entries[rand.nextInt(entries.length)];
- result.add(s);
+ Set<Integer> usedPos = null;
+ if (!duplicatesAllowed) {
+ usedPos = new HashSet<>(count);
+ }
+
+ for (int i = 0; i < count; i++) {
+ int randNum = rand.nextInt(memberMapSize);
+ byte[] member = members.getKeyAtIndex(randNum);
+
+ while (member == null || (!duplicatesAllowed && usedPos.contains(randNum))) {
+ // TODO: Can a member be null?
+ if (!duplicatesAllowed && member == null) {
+ usedPos.add(randNum);
+ }
+ randNum = rand.nextInt(memberMapSize);
+ member = members.getKeyAtIndex(randNum);
+ }
+
+ randMembers.add(members.getKeyAtIndex(randNum));
Review comment:
I think this can be simplified to "randMembers.add(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