[
https://issues.apache.org/jira/browse/GEODE-8059?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17097150#comment-17097150
]
ASF GitHub Bot commented on GEODE-8059:
---------------------------------------
prettyClouds commented on a change in pull request #5035:
URL: https://github.com/apache/geode/pull/5035#discussion_r418405425
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java
##########
@@ -11,40 +11,55 @@
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
* or implied. See the License for the specific language governing permissions
and limitations under
* the License.
+ *
*/
-package org.apache.geode.redis.internal.executor.set;
+package org.apache.geode.redis.internal.executor;
import java.util.ArrayList;
-import java.util.concurrent.atomic.AtomicBoolean;
import org.apache.geode.cache.Region;
import org.apache.geode.cache.execute.Function;
import org.apache.geode.cache.execute.FunctionContext;
import org.apache.geode.cache.execute.ResultSender;
import org.apache.geode.internal.cache.execute.RegionFunctionContextImpl;
import org.apache.geode.redis.internal.ByteArrayWrapper;
+import org.apache.geode.redis.internal.RedisCommandType;
+import org.apache.geode.redis.internal.executor.set.DeltaSet;
-class SremFunction implements Function<ArrayList<ByteArrayWrapper>> {
+public class CommandFunction implements Function<Object[]> {
- public static final String ID = "SREM_FUNCTION";
+ public static final String ID = "REDIS_COMMAND_FUNCTION";
@SuppressWarnings("unchecked")
@Override
- public void execute(FunctionContext<ArrayList<ByteArrayWrapper>> context) {
+ public void execute(FunctionContext<Object[]> context) {
RegionFunctionContextImpl regionFunctionContext =
(RegionFunctionContextImpl) context;
ByteArrayWrapper key =
(ByteArrayWrapper) regionFunctionContext.getFilter().iterator().next();
Region<ByteArrayWrapper, DeltaSet> localRegion =
regionFunctionContext.getLocalDataSet(regionFunctionContext.getDataSet());
- ArrayList<ByteArrayWrapper> membersToRemove =
- (ArrayList<ByteArrayWrapper>) regionFunctionContext.getArguments();
- AtomicBoolean setWasDeleted = new AtomicBoolean();
- long membersRemoved = DeltaSet.srem(localRegion, key, membersToRemove,
setWasDeleted);
- ResultSender<Long> resultSender = regionFunctionContext.getResultSender();
- resultSender.sendResult(membersRemoved);
- resultSender.lastResult(setWasDeleted.get() ? 1L : 0L);
+ ResultSender resultSender = regionFunctionContext.getResultSender();
+ Object[] args = context.getArguments();
+ RedisCommandType command = (RedisCommandType) args[0];
+ ArrayList<ByteArrayWrapper> membersToAdd = (ArrayList<ByteArrayWrapper>)
args[1];
+ switch (command) {
+ case SADD:
Review comment:
I love the use of the `RedisCommandType` here
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/executor/set/DeltaSet.java
##########
@@ -30,10 +30,37 @@
import org.apache.geode.Delta;
import org.apache.geode.InvalidDeltaException;
import org.apache.geode.cache.Region;
+import org.apache.geode.cache.execute.ResultSender;
import org.apache.geode.redis.internal.ByteArrayWrapper;
public class DeltaSet implements Delta, DataSerializable {
+ public static void sadd(ResultSender<Long> resultSender,
+ Region<ByteArrayWrapper, DeltaSet> localRegion, ByteArrayWrapper key,
+ ArrayList<ByteArrayWrapper> membersToAdd) {
+ resultSender.lastResult(sadd(localRegion, key, membersToAdd));
Review comment:
I think it's odd that the Function abstraction bleeds into DeltaSet via
the ResultSender. It restricts DeltaSet to only run in the context of
functions, and adds more responsibility to the class. What are your thoughts?
Some alternatives
Some kind of Decorator Pattern to separate the responsibilities, but still
keep the function code really simple
passing in a more generic consumer into the DeltaSet...that would provide
some isolation, but allow for this more asynchronous style of programming. We
could define the consumer in the function, so the resultsender object stays
encapsulated in the function.
##########
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);
Review comment:
this line is duplicated in each function. I don't think the
`resultcollector` abstraction really makes sense in our case, because we are
really trying to just call this on one server and get back one result. Can we
short circuit this by just calling this `results.getResult().get(0)` in
`executeFunction()`
##########
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:
Could we just send back an object like SremResult? this code is
dependent on ordering of calls to "sendResult" when it doesn't need to be.
----------------------------------------------------------------
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]
> 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)
