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();
```
then after the removeAll operation, check to see if members is empty. If so,
set it back to the BASE_REDIS_SET_OVERHEAD. Since `membersRemoveAll` is only
used in the `applyDelta` method, another option could be checking if
`removes.size()` is equal to `members.size()` before looping through and
subtracting the size of each member. If so, we could set it back to
`BASE_REDIS_SET_OVERHEAD` and return at that point.... though I'm not sure if
this will ever happen since the key is removed from the region altogether and
no delta is set if the set is empty.
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]
