dschneider-pivotal commented on a change in pull request #7228:
URL: https://github.com/apache/geode/pull/7228#discussion_r777676361
##########
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);
Review comment:
would "randomMembers" be a better name? Or maybe even better "result"?
##########
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) {
Review comment:
I think you can remove this "if" block. If member is null we don't add
it to the result. By adding the randNum that is an index to "null" you make the
usedPos set bigger but if in the future we get the same value in "randNum" we
still also call "getKeyAtIndex" and it will return "null". So in the while loop
we see "member == null" is true so we don't even do the usedPos.contains check.
So I think it would be better to get rid of this "if" block and only add to
usedPos when we actually add to "randMembers" which we already do down on line
195.
--
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]
