[
https://issues.apache.org/jira/browse/GEODE-8114?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17106677#comment-17106677
]
ASF GitHub Bot commented on GEODE-8114:
---------------------------------------
prettyClouds commented on a change in pull request #5100:
URL: https://github.com/apache/geode/pull/5100#discussion_r424736683
##########
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:
I can see both sides of this coin.
I think it's nice that the `RedisSetCommands` match the Redis API.
I also think that once we are in the executor we are in the Java code, we
are now doing OO programming and don't need our interfaces to conform to Redis'
binary protocol.
The "user" I imagine implementing this interface is a developer looking to
implement the Redis data structures in a completely new way...in geode or in
some other platform. I don't have enough of that architecture worked out to be
able to make a clear decision, so at this point I am okay either way.
----------------------------------------------------------------
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
> Redis: refactor set classes to implement RedisSetCommands
> ---------------------------------------------------------
>
> Key: GEODE-8114
> URL: https://issues.apache.org/jira/browse/GEODE-8114
> Project: Geode
> Issue Type: Improvement
> Reporter: Murtuza Boxwala
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
