[GitHub] [geode] upthewaterspout commented on a change in pull request #6487: GEODE-9222: Remove ByteArrayWrapper from RedisSet

[GitBox <gi...@apache.org>]

upthewaterspout commented on a change in pull request #6487:
URL: https://github.com/apache/geode/pull/6487#discussion_r634687946



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSet.java
##########
@@ -62,21 +65,19 @@ public int scard() {
   }
 
   @Override
-  long sadd(ArrayList<ByteArrayWrapper> membersToAdd, Region<RedisKey, RedisData> region,
-      RedisKey key) {
+  long sadd(List<byte[]> membersToAdd, Region<RedisKey, RedisData> region, RedisKey key) {
     region.create(key, new RedisSet(membersToAdd));
     return membersToAdd.size();
   }
 
   @Override
-  long srem(ArrayList<ByteArrayWrapper> membersToRemove, Region<RedisKey, RedisData> region,
-      RedisKey key) {
+  long srem(List<byte[]> membersToRemove, Region<RedisKey, RedisData> region, RedisKey key) {
     return 0;
   }
 
   @Override
   @VisibleForTesting
-  Set<ByteArrayWrapper> smembers() {
+  public Set<byte[]> smembers() {
     // some callers want to be able to modify the set returned
     return new HashSet<>();

Review comment:
       Should this be an Object2Object... set instead of a HashMap<>? If the callers modify this returned set, they won't be getting the right hashing behavior.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -340,7 +306,7 @@ long srem(ArrayList<ByteArrayWrapper> membersToRemove, Region<RedisKey, RedisDat
    * @return a set containing all the members in this set
    */
   @VisibleForTesting
-  Set<ByteArrayWrapper> smembers() {
+  public Set<byte[]> smembers() {
     return new HashSet<>(members);

Review comment:
       This definitely needs to be an Object2ObjectOpenCustomHashMap, not a HashMap here.

##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisSetTest.java
##########
@@ -193,9 +197,10 @@ public void should_calculateSize_equalToROS_withNoMembers() {
   }
 
   @Test
+  @Ignore("Sizing tests are known to be flaky/incorrect and will be fixed as part of GEODE-9279")
   public void should_calculateSize_equalToROS_withSingleMember() {
-    HashSet<ByteArrayWrapper> members = new HashSet<>();
-    members.add(new ByteArrayWrapper("value".getBytes()));
+    Set<byte[]> members = new HashSet<>();

Review comment:
       I guess these tests are technically correct, but it still seems weird to use a HashSet<byte[]>() here. That will use the Object.hashCode() of the byte[] array. Maybe make this a ArrayList<> instead to make it clear we don't really care about set semantics on `members` here? Or use Object2ObjectOpenCustomHashMap instead.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org