dschneider-pivotal commented on a change in pull request #5185:
URL: https://github.com/apache/geode/pull/5185#discussion_r432720582
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
##########
@@ -107,132 +107,161 @@
*/
public enum RedisCommandType {
+ /***************************************
+ *** Supported Commands ***
+ ***************************************/
+
+ /*************** Keys ******************/
+
+ AUTH(new AuthExecutor(), true),
+ DEL(new DelExecutor(), true, new MinimumParameterRequirements(2)),
+ EXISTS(new ExistsExecutor(), true, new MinimumParameterRequirements(2)),
+ EXPIRE(new ExpireExecutor(), true),
+ EXPIREAT(new ExpireAtExecutor(), true),
+ FLUSHALL(new FlushAllExecutor(), true),
+ KEYS(new KeysExecutor(), true),
+ PEXPIRE(new PExpireExecutor(), true),
+ PEXPIREAT(new PExpireAtExecutor(), true),
+ RENAME(new RenameExecutor(), true),
+
+ /************* Strings *****************/
+
+ APPEND(new AppendExecutor(), true, new ExactParameterRequirements(3)),
+ GET(new GetExecutor(), true, new ExactParameterRequirements(2)),
+ SET(new SetExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /************* Hashes *****************/
+
+ HGETALL(new HGetAllExecutor(), true, new ExactParameterRequirements(2)),
+ HMSET(new HMSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+ HSET(new HSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+
+ /************* Sets *****************/
+
+ SADD(new SAddExecutor(), true, new MinimumParameterRequirements(3)),
+ SMEMBERS(new SMembersExecutor(), true, new ExactParameterRequirements(2)),
+ SREM(new SRemExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /********** Publish Subscribe **********/
+
+ SUBSCRIBE(new SubscribeExecutor(), true),
+ PUBLISH(new PublishExecutor(), true),
+ UNSUBSCRIBE(new UnsubscribeExecutor(), true),
+ PSUBSCRIBE(new PsubscribeExecutor(), true),
+ PUNSUBSCRIBE(new PunsubscribeExecutor(), true),
+
+ /*************** Server ****************/
+
+ PING(new PingExecutor(), true),
+ QUIT(new QuitExecutor(), true),
+ UNKNOWN(new UnknownExecutor(), true),
+
+
+ /***************************************
+ *** Unsupported Commands ***
+ ***************************************/
+
/***************************************
*************** Keys ******************
***************************************/
- AUTH(new AuthExecutor()),
- DEL(new DelExecutor(), new MinimumParameterRequirements(2)),
- EXISTS(new ExistsExecutor(), new MinimumParameterRequirements(2)),
- EXPIRE(new ExpireExecutor()),
- EXPIREAT(new ExpireAtExecutor()),
- FLUSHALL(new FlushAllExecutor()),
- FLUSHDB(new FlushAllExecutor()),
- KEYS(new KeysExecutor()),
- PERSIST(new PersistExecutor()),
- PEXPIRE(new PExpireExecutor()),
- PEXPIREAT(new PExpireAtExecutor()),
- PTTL(new PTTLExecutor()),
- RENAME(new RenameExecutor()),
- SCAN(new ScanExecutor()),
- TTL(new TTLExecutor()),
- TYPE(new TypeExecutor()),
+ FLUSHDB(new FlushAllExecutor(), false),
+ PERSIST(new PersistExecutor(), false),
Review comment:
PERSIST should be supported
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/ExecutionHandlerContext.java
##########
@@ -196,22 +197,26 @@ public void channelInactive(ChannelHandlerContext ctx) {
private void executeCommand(ChannelHandlerContext ctx, Command command)
throws Exception {
RedisResponse response;
- if (isAuthenticated) {
- if (command.isOfType(RedisCommandType.SHUTDOWN)) {
- this.server.shutdown();
- return;
- }
- response = command.execute(this);
+ if (command.isOfType(RedisCommandType.SHUTDOWN)) {
Review comment:
Should this SHUTDOWN command code be moved to after we check of
unsupported commands are allowed? I think SHUTDOWN is unsupported
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
##########
@@ -107,132 +107,161 @@
*/
public enum RedisCommandType {
+ /***************************************
+ *** Supported Commands ***
+ ***************************************/
+
+ /*************** Keys ******************/
+
+ AUTH(new AuthExecutor(), true),
+ DEL(new DelExecutor(), true, new MinimumParameterRequirements(2)),
+ EXISTS(new ExistsExecutor(), true, new MinimumParameterRequirements(2)),
+ EXPIRE(new ExpireExecutor(), true),
+ EXPIREAT(new ExpireAtExecutor(), true),
+ FLUSHALL(new FlushAllExecutor(), true),
+ KEYS(new KeysExecutor(), true),
+ PEXPIRE(new PExpireExecutor(), true),
+ PEXPIREAT(new PExpireAtExecutor(), true),
+ RENAME(new RenameExecutor(), true),
+
+ /************* Strings *****************/
+
+ APPEND(new AppendExecutor(), true, new ExactParameterRequirements(3)),
+ GET(new GetExecutor(), true, new ExactParameterRequirements(2)),
+ SET(new SetExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /************* Hashes *****************/
+
+ HGETALL(new HGetAllExecutor(), true, new ExactParameterRequirements(2)),
+ HMSET(new HMSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+ HSET(new HSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+
+ /************* Sets *****************/
+
+ SADD(new SAddExecutor(), true, new MinimumParameterRequirements(3)),
+ SMEMBERS(new SMembersExecutor(), true, new ExactParameterRequirements(2)),
+ SREM(new SRemExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /********** Publish Subscribe **********/
+
+ SUBSCRIBE(new SubscribeExecutor(), true),
+ PUBLISH(new PublishExecutor(), true),
+ UNSUBSCRIBE(new UnsubscribeExecutor(), true),
+ PSUBSCRIBE(new PsubscribeExecutor(), true),
+ PUNSUBSCRIBE(new PunsubscribeExecutor(), true),
+
+ /*************** Server ****************/
+
+ PING(new PingExecutor(), true),
+ QUIT(new QuitExecutor(), true),
+ UNKNOWN(new UnknownExecutor(), true),
+
+
+ /***************************************
+ *** Unsupported Commands ***
+ ***************************************/
+
/***************************************
*************** Keys ******************
***************************************/
- AUTH(new AuthExecutor()),
- DEL(new DelExecutor(), new MinimumParameterRequirements(2)),
- EXISTS(new ExistsExecutor(), new MinimumParameterRequirements(2)),
- EXPIRE(new ExpireExecutor()),
- EXPIREAT(new ExpireAtExecutor()),
- FLUSHALL(new FlushAllExecutor()),
- FLUSHDB(new FlushAllExecutor()),
- KEYS(new KeysExecutor()),
- PERSIST(new PersistExecutor()),
- PEXPIRE(new PExpireExecutor()),
- PEXPIREAT(new PExpireAtExecutor()),
- PTTL(new PTTLExecutor()),
- RENAME(new RenameExecutor()),
- SCAN(new ScanExecutor()),
- TTL(new TTLExecutor()),
- TYPE(new TypeExecutor()),
+ FLUSHDB(new FlushAllExecutor(), false),
+ PERSIST(new PersistExecutor(), false),
+ PTTL(new PTTLExecutor(), false),
+ SCAN(new ScanExecutor(), false),
+ TTL(new TTLExecutor(), false),
+ TYPE(new TypeExecutor(), false),
Review comment:
TYPE should be supported
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
##########
@@ -107,132 +107,161 @@
*/
public enum RedisCommandType {
+ /***************************************
+ *** Supported Commands ***
+ ***************************************/
+
+ /*************** Keys ******************/
+
+ AUTH(new AuthExecutor(), true),
+ DEL(new DelExecutor(), true, new MinimumParameterRequirements(2)),
+ EXISTS(new ExistsExecutor(), true, new MinimumParameterRequirements(2)),
+ EXPIRE(new ExpireExecutor(), true),
+ EXPIREAT(new ExpireAtExecutor(), true),
+ FLUSHALL(new FlushAllExecutor(), true),
+ KEYS(new KeysExecutor(), true),
+ PEXPIRE(new PExpireExecutor(), true),
+ PEXPIREAT(new PExpireAtExecutor(), true),
+ RENAME(new RenameExecutor(), true),
+
+ /************* Strings *****************/
+
+ APPEND(new AppendExecutor(), true, new ExactParameterRequirements(3)),
+ GET(new GetExecutor(), true, new ExactParameterRequirements(2)),
+ SET(new SetExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /************* Hashes *****************/
+
+ HGETALL(new HGetAllExecutor(), true, new ExactParameterRequirements(2)),
+ HMSET(new HMSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+ HSET(new HSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+
+ /************* Sets *****************/
+
+ SADD(new SAddExecutor(), true, new MinimumParameterRequirements(3)),
+ SMEMBERS(new SMembersExecutor(), true, new ExactParameterRequirements(2)),
+ SREM(new SRemExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /********** Publish Subscribe **********/
+
+ SUBSCRIBE(new SubscribeExecutor(), true),
+ PUBLISH(new PublishExecutor(), true),
+ UNSUBSCRIBE(new UnsubscribeExecutor(), true),
+ PSUBSCRIBE(new PsubscribeExecutor(), true),
+ PUNSUBSCRIBE(new PunsubscribeExecutor(), true),
+
+ /*************** Server ****************/
+
+ PING(new PingExecutor(), true),
+ QUIT(new QuitExecutor(), true),
+ UNKNOWN(new UnknownExecutor(), true),
+
+
+ /***************************************
+ *** Unsupported Commands ***
+ ***************************************/
+
/***************************************
*************** Keys ******************
***************************************/
- AUTH(new AuthExecutor()),
- DEL(new DelExecutor(), new MinimumParameterRequirements(2)),
- EXISTS(new ExistsExecutor(), new MinimumParameterRequirements(2)),
- EXPIRE(new ExpireExecutor()),
- EXPIREAT(new ExpireAtExecutor()),
- FLUSHALL(new FlushAllExecutor()),
- FLUSHDB(new FlushAllExecutor()),
- KEYS(new KeysExecutor()),
- PERSIST(new PersistExecutor()),
- PEXPIRE(new PExpireExecutor()),
- PEXPIREAT(new PExpireAtExecutor()),
- PTTL(new PTTLExecutor()),
- RENAME(new RenameExecutor()),
- SCAN(new ScanExecutor()),
- TTL(new TTLExecutor()),
- TYPE(new TypeExecutor()),
+ FLUSHDB(new FlushAllExecutor(), false),
+ PERSIST(new PersistExecutor(), false),
+ PTTL(new PTTLExecutor(), false),
+ SCAN(new ScanExecutor(), false),
+ TTL(new TTLExecutor(), false),
Review comment:
TTL should be supported
##########
File path:
geode-redis/src/test/java/org/apache/geode/redis/internal/SupportedCommandsJUnitTest.java
##########
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.junit.Test;
+
+public class SupportedCommandsJUnitTest {
+
+ private final String[] supportedCommands = new String[] {
Review comment:
This seems painful but might be okay. I just don't like that we have to
update two places (the enum and these lists) anytime we change what we support.
But it probably will not happen that often.
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
##########
@@ -107,132 +107,161 @@
*/
public enum RedisCommandType {
+ /***************************************
+ *** Supported Commands ***
+ ***************************************/
+
+ /*************** Keys ******************/
+
+ AUTH(new AuthExecutor(), true),
+ DEL(new DelExecutor(), true, new MinimumParameterRequirements(2)),
+ EXISTS(new ExistsExecutor(), true, new MinimumParameterRequirements(2)),
+ EXPIRE(new ExpireExecutor(), true),
+ EXPIREAT(new ExpireAtExecutor(), true),
+ FLUSHALL(new FlushAllExecutor(), true),
+ KEYS(new KeysExecutor(), true),
+ PEXPIRE(new PExpireExecutor(), true),
+ PEXPIREAT(new PExpireAtExecutor(), true),
+ RENAME(new RenameExecutor(), true),
+
+ /************* Strings *****************/
+
+ APPEND(new AppendExecutor(), true, new ExactParameterRequirements(3)),
+ GET(new GetExecutor(), true, new ExactParameterRequirements(2)),
+ SET(new SetExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /************* Hashes *****************/
+
+ HGETALL(new HGetAllExecutor(), true, new ExactParameterRequirements(2)),
+ HMSET(new HMSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+ HSET(new HSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+
+ /************* Sets *****************/
+
+ SADD(new SAddExecutor(), true, new MinimumParameterRequirements(3)),
+ SMEMBERS(new SMembersExecutor(), true, new ExactParameterRequirements(2)),
+ SREM(new SRemExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /********** Publish Subscribe **********/
+
+ SUBSCRIBE(new SubscribeExecutor(), true),
+ PUBLISH(new PublishExecutor(), true),
+ UNSUBSCRIBE(new UnsubscribeExecutor(), true),
+ PSUBSCRIBE(new PsubscribeExecutor(), true),
+ PUNSUBSCRIBE(new PunsubscribeExecutor(), true),
+
+ /*************** Server ****************/
+
+ PING(new PingExecutor(), true),
+ QUIT(new QuitExecutor(), true),
+ UNKNOWN(new UnknownExecutor(), true),
+
+
+ /***************************************
+ *** Unsupported Commands ***
+ ***************************************/
+
/***************************************
*************** Keys ******************
***************************************/
- AUTH(new AuthExecutor()),
- DEL(new DelExecutor(), new MinimumParameterRequirements(2)),
- EXISTS(new ExistsExecutor(), new MinimumParameterRequirements(2)),
- EXPIRE(new ExpireExecutor()),
- EXPIREAT(new ExpireAtExecutor()),
- FLUSHALL(new FlushAllExecutor()),
- FLUSHDB(new FlushAllExecutor()),
- KEYS(new KeysExecutor()),
- PERSIST(new PersistExecutor()),
- PEXPIRE(new PExpireExecutor()),
- PEXPIREAT(new PExpireAtExecutor()),
- PTTL(new PTTLExecutor()),
- RENAME(new RenameExecutor()),
- SCAN(new ScanExecutor()),
- TTL(new TTLExecutor()),
- TYPE(new TypeExecutor()),
+ FLUSHDB(new FlushAllExecutor(), false),
+ PERSIST(new PersistExecutor(), false),
+ PTTL(new PTTLExecutor(), false),
Review comment:
PTTL should be supported
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java
##########
@@ -107,132 +107,161 @@
*/
public enum RedisCommandType {
+ /***************************************
+ *** Supported Commands ***
+ ***************************************/
+
+ /*************** Keys ******************/
+
+ AUTH(new AuthExecutor(), true),
+ DEL(new DelExecutor(), true, new MinimumParameterRequirements(2)),
+ EXISTS(new ExistsExecutor(), true, new MinimumParameterRequirements(2)),
+ EXPIRE(new ExpireExecutor(), true),
+ EXPIREAT(new ExpireAtExecutor(), true),
+ FLUSHALL(new FlushAllExecutor(), true),
+ KEYS(new KeysExecutor(), true),
+ PEXPIRE(new PExpireExecutor(), true),
+ PEXPIREAT(new PExpireAtExecutor(), true),
+ RENAME(new RenameExecutor(), true),
+
+ /************* Strings *****************/
+
+ APPEND(new AppendExecutor(), true, new ExactParameterRequirements(3)),
+ GET(new GetExecutor(), true, new ExactParameterRequirements(2)),
+ SET(new SetExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /************* Hashes *****************/
+
+ HGETALL(new HGetAllExecutor(), true, new ExactParameterRequirements(2)),
+ HMSET(new HMSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+ HSET(new HSetExecutor(), true,
+ new MinimumParameterRequirements(4).and(new
EvenParameterRequirements())),
+
+ /************* Sets *****************/
+
+ SADD(new SAddExecutor(), true, new MinimumParameterRequirements(3)),
+ SMEMBERS(new SMembersExecutor(), true, new ExactParameterRequirements(2)),
+ SREM(new SRemExecutor(), true, new MinimumParameterRequirements(3)),
+
+ /********** Publish Subscribe **********/
+
+ SUBSCRIBE(new SubscribeExecutor(), true),
+ PUBLISH(new PublishExecutor(), true),
+ UNSUBSCRIBE(new UnsubscribeExecutor(), true),
+ PSUBSCRIBE(new PsubscribeExecutor(), true),
+ PUNSUBSCRIBE(new PunsubscribeExecutor(), true),
+
+ /*************** Server ****************/
+
+ PING(new PingExecutor(), true),
+ QUIT(new QuitExecutor(), true),
+ UNKNOWN(new UnknownExecutor(), true),
+
+
+ /***************************************
+ *** Unsupported Commands ***
+ ***************************************/
+
/***************************************
*************** Keys ******************
***************************************/
- AUTH(new AuthExecutor()),
- DEL(new DelExecutor(), new MinimumParameterRequirements(2)),
- EXISTS(new ExistsExecutor(), new MinimumParameterRequirements(2)),
- EXPIRE(new ExpireExecutor()),
- EXPIREAT(new ExpireAtExecutor()),
- FLUSHALL(new FlushAllExecutor()),
- FLUSHDB(new FlushAllExecutor()),
- KEYS(new KeysExecutor()),
- PERSIST(new PersistExecutor()),
- PEXPIRE(new PExpireExecutor()),
- PEXPIREAT(new PExpireAtExecutor()),
- PTTL(new PTTLExecutor()),
- RENAME(new RenameExecutor()),
- SCAN(new ScanExecutor()),
- TTL(new TTLExecutor()),
- TYPE(new TypeExecutor()),
+ FLUSHDB(new FlushAllExecutor(), false),
Review comment:
Since we support FLUSHALL and FLUSHDB uses the FlushAllExecutor should
we support it?
----------------------------------------------------------------
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
