dschneider-pivotal commented on a change in pull request #5100:
URL: https://github.com/apache/geode/pull/5100#discussion_r424728367
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/RedisSet.java
##########
@@ -304,70 +211,60 @@ public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
}
/**
- * @param membersToAdd members to add to this set; NOTE this list may by
- * modified by this call
- * @param region the region this instance is stored in
* @param key the name of the set to add to
+ * @param membersToAdd members to add to this set; NOTE this list may by
modified by this call
* @return the number of members actually added; -1 if concurrent
modification
*/
- private synchronized long doSadd(ArrayList<ByteArrayWrapper> membersToAdd,
- Region<ByteArrayWrapper, RedisSet> region,
- ByteArrayWrapper key) {
-
+ @Override
+ public synchronized long sadd(ByteArrayWrapper key,
ArrayList<ByteArrayWrapper> membersToAdd) {
+ this.deltas = null;
membersToAdd.removeIf(memberToAdd -> !members.add(memberToAdd));
int membersAdded = membersToAdd.size();
if (membersAdded != 0) {
deltasAreAdds = true;
deltas = membersToAdd;
- try {
- region.put(key, this);
- } finally {
- deltas = null;
- }
}
return membersAdded;
}
/**
- * @param membersToRemove members to remove from this set; NOTE this list
may by
- * modified by this call
- * @param region the region this instance is stored in
* @param key the name of the set to remove from
+ * @param membersToRemove members to remove from this set; NOTE this list
may by modified by this
+ * call
* @param setWasDeleted set to true if this method deletes the set
* @return the number of members actually removed; -1 if concurrent
modification
*/
- private synchronized long doSrem(ArrayList<ByteArrayWrapper> membersToRemove,
- Region<ByteArrayWrapper, RedisSet> region,
- ByteArrayWrapper key, AtomicBoolean setWasDeleted) {
-
+ @Override
+ public synchronized long srem(ByteArrayWrapper key,
ArrayList<ByteArrayWrapper> membersToRemove,
+ AtomicBoolean setWasDeleted) {
+ deltas = null;
Review comment:
When JohnH and I worked on renaming RedisSetCommands (from RedisSet), we
chose "Commands" because it seemed like a bag of functions. And from the point
of view of the netty executors (like SAddExecutor) I think this may make sense.
I like for the interface methods to have a close correspondence to the actual
redis command line arguments. Since the REDIS SADD takes a "key" and "members"
then the method on the Commands interface should have both a "key" and
"members". It should not have "region" since the REDIS SADD command does not
have a region. So that is why when you create a
RedisSetCommandsFunctionExecutor you give it a Region but you do not give it
the key. This also means you could use the same instance of
RedisSetCommandsFunctionExecutor to do multiple commands on different keys.
But once you get down to having on instance of our data structure (RedisSet
in this example) then you no longer need the key because the key was used to
find it.
If I had to choose one or the other (methods with the key or methods
without), I think I'd chose with the key since it corresponds to the redis
apis.
----------------------------------------------------------------
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:
us...@infra.apache.org
