Kris-10-0 commented on a change in pull request #7228:
URL: https://github.com/apache/geode/pull/7228#discussion_r780464930
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -202,48 +202,50 @@ static int setOpStoreResult(RegionProvider
regionProvider, RedisKey destinationK
return popped;
}
- public Collection<byte[]> srandmember(int count) {
- int membersSize = members.size();
- boolean duplicatesAllowed = count < 0;
- if (duplicatesAllowed) {
- count = -count;
- }
+ public List<byte[]> srandmember(int count) {
+ boolean uniqueNumberList = count > 0;
+ count = uniqueNumberList ? count : -count;
+ int memberMapSize = members.getMemberMapSize();
- if (!duplicatesAllowed && membersSize <= count && count != 1) {
- return new ArrayList<>(members);
+ // TODO: Optomize algorithm
+ List<byte[]> result = new ArrayList<>(count);
+ if (uniqueNumberList) {
+ if (count >= members.size()) {
+ result.addAll(members);
+ } else {
+ srandomUniqueList(count, memberMapSize, result);
+ }
+ } else {
+ srandomDuplicateList(count, memberMapSize, result);
}
+ return result;
+ }
- Random rand = new Random();
+ private void srandomDuplicateList(int count, int memberMapSize, List<byte[]>
result) {
+ while (result.size() != count) {
+ int randIndex = rand.nextInt(memberMapSize);
+ byte[] member = members.getKeyAtIndex(randIndex);
- // 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--;
+ if (member != null) {
+ result.add(member);
}
- return result;
- } 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);
+ }
+ }
+
+ private void srandomUniqueList(int count, int memberMapSize, List<byte[]>
result) {
+ List<Integer> allIndexes = new ArrayList<>(memberMapSize);
+ for (int i = 0; i < memberMapSize; i++) {
+ allIndexes.add(i);
+ }
+ Collections.shuffle(allIndexes);
+ int i = 0;
+ while (result.size() != count) {
+ byte[] member = members.getKeyAtIndex(i);
Review comment:
I think I was worried since getKey(pos) returned an object K it wouldn't
be a byte[] when I called it directly. I tried the direct call and it worked.
So I'll make the fix.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSetsIntegrationTest.java
##########
@@ -98,67 +97,6 @@ public void testSAdd_canStoreBinaryData() {
assertThat(result).containsExactly(blob);
}
- @Test
- public void srandmember_withStringFails() {
- jedis.set("string", "value");
- assertThatThrownBy(() ->
jedis.srandmember("string")).hasMessageContaining("WRONGTYPE");
- }
-
- @Test
- public void srandmember_withNonExistentKeyReturnsNull() {
- assertThat(jedis.srandmember("non existent")).isNull();
- }
-
- @Test
- public void srandmemberCount_withNonExistentKeyReturnsEmptyArray() {
- assertThat(jedis.srandmember("non existent", 3)).isEmpty();
- }
-
- @Test
- public void srandmember_returnsOneMember() {
- jedis.sadd("key", "m1", "m2");
- String result = jedis.srandmember("key");
- assertThat(result).isIn("m1", "m2");
- }
-
- @Test
- public void srandmemberCount_returnsTwoUniqueMembers() {
- jedis.sadd("key", "m1", "m2", "m3");
- List<String> results = jedis.srandmember("key", 2);
- assertThat(results).hasSize(2);
- assertThat(results).containsAnyOf("m1", "m2", "m3");
- assertThat(results.get(0)).isNotEqualTo(results.get(1));
- }
-
- @Test
- public void srandmemberNegativeCount_returnsThreeMembers() {
- jedis.sadd("key", "m1", "m2", "m3");
- List<String> results = jedis.srandmember("key", -3);
- assertThat(results).hasSize(3);
- assertThat(results).containsAnyOf("m1", "m2", "m3");
- }
-
- @Test
- public void srandmemberNegativeCount_givenSmallSet_returnsThreeMembers() {
- jedis.sadd("key", "m1");
- List<String> results = jedis.srandmember("key", -3);
- assertThat(results).hasSize(3);
- assertThat(results).containsAnyOf("m1");
- }
-
- @Test
- public void
smembers_givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key",
Protocol.Command.SMEMBERS))
- .hasMessageContaining("ERR wrong number of arguments for 'smembers'
command");
- }
-
- @Test
- public void
smembers_givenMoreThanTwoArguments_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis
- .sendCommand("key", Protocol.Command.SMEMBERS, "key", "extraArg"))
- .hasMessageContaining("ERR wrong number of arguments for
'smembers' command");
- }
Review comment:
Nope accident.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -161,49 +162,38 @@ 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();
Review comment:
@DonalEvans Creating a final field for the class messes with the size of
the bytes causing tests in RedisSet to fail.
--
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]
