[GitHub] [geode] dschneider-pivotal commented on a change in pull request #4989: GEODE 8014: Sets and Hashes should be deleted once they are empty

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

dschneider-pivotal commented on a change in pull request #4989:
URL: https://github.com/apache/geode/pull/4989#discussion_r413992483



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java
##########
@@ -89,6 +89,11 @@ public int hdel(List<ByteArrayWrapper> subList) {
       numDeleted.set(oldHash.size() - newHash.size());
       return newHash;
     });
+
+    if (hgetall().size() == 0) {

Review comment:
       Calling "hgetall" and then checking the size does not seem thread safe. Couldn't another  thread add to this set and find the existing one we emptied out and add to it right after this thread checks the size and then we will unregister a non-empty hash?
   If we do have a concurrency issue I'm still okay with this change since it fixes the non-concurrent case. Maybe we could just add a TODO comment that concurrency needs to be addressed?

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java
##########
@@ -89,6 +89,11 @@ public int hdel(List<ByteArrayWrapper> subList) {
       numDeleted.set(oldHash.size() - newHash.size());
       return newHash;
     });
+
+    if (hgetall().size() == 0) {

Review comment:
       Instead of ".size() == 0" I think ".isEmpty()" is better.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/GeodeRedisHashSynchronized.java
##########
@@ -89,6 +89,11 @@ public int hdel(List<ByteArrayWrapper> subList) {
       numDeleted.set(oldHash.size() - newHash.size());
       return newHash;
     });
+
+    if (hgetall().size() == 0) {
+      RedisDataType type = context.getKeyRegistrar().getType(key);

Review comment:
       It seems like this code should only unregister if the current type is REDIS_HASH. I thought you were unregistering here but does getRegionProvider().removeKey change the keyRegistar? I will talk with Sarah about this. I might just not understand this code

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/GeodeRedisSetSynchronized.java
##########
@@ -59,8 +60,15 @@ public long srem(Collection<ByteArrayWrapper> membersToRemove) {
       Set<ByteArrayWrapper> newValue = createSet(oldValue);
       newValue.removeAll(membersToRemove);
       removedCount.set(oldValue.size() - newValue.size());
+
       return newValue;
     });
+
+    if (members().size() == 0) {

Review comment:
       Same comments on this block as the above hdel




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