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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
