sabbey37 commented on a change in pull request #6296:
URL: https://github.com/apache/geode/pull/6296#discussion_r628499173
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -243,24 +251,28 @@ private boolean membersRemove(ByteArrayWrapper
memberToRemove) {
boolean isRemoved = members.remove(memberToRemove);
if (isRemoved) {
sizeInBytes -= PER_MEMBER_OVERHEAD + memberToRemove.length();
+ if (members.isEmpty()) {
+ sizeInBytes = BASE_REDIS_SET_OVERHEAD;
+ }
}
return isRemoved;
}
- private synchronized boolean membersAddAll(AddsDeltaInfo addsDeltaInfo) {
+ private synchronized void membersAddAll(AddsDeltaInfo addsDeltaInfo) {
ArrayList<ByteArrayWrapper> adds = addsDeltaInfo.getAdds();
sizeInBytes += adds.stream().mapToInt(a -> a.length() +
PER_MEMBER_OVERHEAD).sum();
- return members.addAll(adds);
+ if (members.size() == 1) {
+ sizeInBytes += INTERNAL_HASH_SET_STORAGE_OVERHEAD;
+ }
+ members.addAll(adds);
}
- private synchronized boolean membersRemoveAll(RemsDeltaInfo remsDeltaInfo) {
+ private synchronized void membersRemoveAll(RemsDeltaInfo remsDeltaInfo) {
ArrayList<ByteArrayWrapper> removes = remsDeltaInfo.getRemoves();
- sizeInBytes -= removes.stream().mapToInt(a -> a.length() +
PER_MEMBER_OVERHEAD).sum();
- return members.removeAll(removes);
+ sizeInBytes = BASE_REDIS_SET_OVERHEAD;
+ members.removeAll(removes);
Review comment:
This method does not remove every member from the set.
members.removeAll() will just remove the elements contained in `removes`, so
the set may not be empty afterwards. With that in mind, it makes sense to keep
the code that was here previously:
```
sizeInBytes -= removes.stream().mapToInt(a -> a.length() +
PER_MEMBER_OVERHEAD).sum();
```
Because this method is only ever called from `applyDelta`, we actually don't
need to check if the set is empty. The set will never be empty in this method
because once the set is empty (on a different server), the key is removed from
the region altogether and no `RedisSet` delta is applied/sent (so this method
would not be called in that scenario).
Maybe we need to improve the way we test this?
--
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:
[email protected]
