[jira] [Commented] (GEODE-8059) Use a single function to route redis commands to primary

[ASF GitHub Bot (Jira)]

    [ 
https://issues.apache.org/jira/browse/GEODE-8059?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17097213#comment-17097213
 ] 

ASF GitHub Bot commented on GEODE-8059:
---------------------------------------

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



##########
File path: 
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/GeodeRedisSetWithFunctions.java
##########
@@ -40,30 +47,18 @@ public GeodeRedisSetWithFunctions(ByteArrayWrapper key,
   }
 
   public static void registerFunctions() {
-    FunctionService.registerFunction(new SaddFunction());
-    FunctionService.registerFunction(new SremFunction());
-    FunctionService.registerFunction(new SmembersFunction());
-    FunctionService.registerFunction(new SdelFunction());
+    FunctionService.registerFunction(new CommandFunction());
   }
 
   @Override
   public long sadd(ArrayList<ByteArrayWrapper> membersToAdd) {
-    ResultCollector<ArrayList<ByteArrayWrapper>, List<Long>> results = 
FunctionService
-        .onRegion(region)
-        .withFilter(Collections.singleton(key))
-        .setArguments(membersToAdd)
-        .execute(SaddFunction.ID);
-
+    ResultCollector<Object[], List<Long>> results = executeFunction(SADD, 
membersToAdd);
     return results.getResult().get(0);
   }
 
   @Override
   public long srem(ArrayList<ByteArrayWrapper> membersToRemove, AtomicBoolean 
setWasDeleted) {
-    ResultCollector<ArrayList<ByteArrayWrapper>, List<Long>> results = 
FunctionService
-        .onRegion(region)
-        .withFilter(Collections.singleton(key))
-        .setArguments(membersToRemove)
-        .execute(SremFunction.ID);
+    ResultCollector<Object[], List<Long>> results = executeFunction(SREM, 
membersToRemove);
     List<Long> resultList = results.getResult();
     long membersRemoved = resultList.get(0);

Review comment:
       I think having srem return two things will not be needed for long. Once 
we have a single region (instead of the metaType region and the dataStore 
region) then we no longer need to tell the caller that the srem did a delete. 
So instead of making a new serialize class (like SremResult) we could just have 
a single result that is an long[] in this case. Then we only need to do 
resultSender.get(0) like the other operations.




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


> Use a single function to route redis commands to primary
> --------------------------------------------------------
>
>                 Key: GEODE-8059
>                 URL: https://issues.apache.org/jira/browse/GEODE-8059
>             Project: Geode
>          Issue Type: Improvement
>          Components: redis
>            Reporter: Darrel Schneider
>            Assignee: Darrel Schneider
>            Priority: Major
>
> The redis implementation now uses a function to route the execution of a 
> redis command to the server that has the primary for a particular key. 
> Currently a function exists for each command (sadd, srem, smembers, del). 
> Each time we want to do this for another redis command it is a bunch of work 
> if we add another function.
> Instead we could have a single function that takes an enum describing what 
> command it is being asked to execute.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)