You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/09/23 16:52:28 UTC
[GitHub] [geode] sabbeyPivotal opened a new pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
sabbeyPivotal opened a new pull request #5544:
URL: https://github.com/apache/geode/pull/5544
From PING documentation (https://redis.io/commands/ping):
If the client is subscribed to a channel or a pattern, it will instead return a multi-bulk with a "pong" in the first position and an empty bulk in the second position, unless an argument is provided in which case it returns a copy of the argument.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #5544:
URL: https://github.com/apache/geode/pull/5544
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5544:
URL: https://github.com/apache/geode/pull/5544#discussion_r494536127
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/PingExecutor.java
##########
@@ -31,9 +32,17 @@ public RedisResponse executeCommand(Command command,
ExecutionHandlerContext context) {
List<byte[]> commandElems = command.getProcessedCommand();
byte[] result = PING_RESPONSE.getBytes();
+ byte[] subscribeResult = "".getBytes();
if (commandElems.size() > 1) {
result = commandElems.get(1);
+ subscribeResult = result;
}
+
+ if (!context.getPubSub().findSubscribedChannels(context.getClient()).isEmpty()) {
Review comment:
I refactored it. Check it out and let me know what you think.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5544:
URL: https://github.com/apache/geode/pull/5544#discussion_r494487359
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/SubscriptionsIntegrationTest.java
##########
@@ -39,9 +38,8 @@
public static ExecutorServiceRule executor = new ExecutorServiceRule();
@Test
- @Ignore("GEODE-8515")
public void pingWhileSubscribed() {
- Jedis client = new Jedis("localhost", server.getPort());
+ Jedis client = new Jedis("localhost", server.getPort(), 1000000000);
Review comment:
why a long timeout on this one but not on pingWithArgumentWhileSubscribed?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/PingExecutor.java
##########
@@ -31,9 +32,17 @@ public RedisResponse executeCommand(Command command,
ExecutionHandlerContext context) {
List<byte[]> commandElems = command.getProcessedCommand();
byte[] result = PING_RESPONSE.getBytes();
+ byte[] subscribeResult = "".getBytes();
if (commandElems.size() > 1) {
result = commandElems.get(1);
+ subscribeResult = result;
}
+
+ if (!context.getPubSub().findSubscribedChannels(context.getClient()).isEmpty()) {
Review comment:
consider refactoring this so that we only compute the RedisResponse for subscribe if we find a client and then otherwise with (an else) compute the RedisResponse when not subscribed. I don't like all the logic we have initializing two different results and then we only end up using one or the other. I'd prefer that we only have state at the top that would be shared by either branch (for example commandElems). Once the RedisResponse is done we can then fall through to a command return.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5544:
URL: https://github.com/apache/geode/pull/5544#discussion_r494487359
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/SubscriptionsIntegrationTest.java
##########
@@ -39,9 +38,8 @@
public static ExecutorServiceRule executor = new ExecutorServiceRule();
@Test
- @Ignore("GEODE-8515")
public void pingWhileSubscribed() {
- Jedis client = new Jedis("localhost", server.getPort());
+ Jedis client = new Jedis("localhost", server.getPort(), 1000000000);
Review comment:
why a long timeout on this one but not on pingWithArgumentWhileSubscribed?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/PingExecutor.java
##########
@@ -31,9 +32,17 @@ public RedisResponse executeCommand(Command command,
ExecutionHandlerContext context) {
List<byte[]> commandElems = command.getProcessedCommand();
byte[] result = PING_RESPONSE.getBytes();
+ byte[] subscribeResult = "".getBytes();
if (commandElems.size() > 1) {
result = commandElems.get(1);
+ subscribeResult = result;
}
+
+ if (!context.getPubSub().findSubscribedChannels(context.getClient()).isEmpty()) {
Review comment:
consider refactoring this so that we only compute the RedisResponse for subscribe if we find a client and then otherwise with (an else) compute the RedisResponse when not subscribed. I don't like all the logic we have initializing two different results and then we only end up using one or the other. I'd prefer that we only have state at the top that would be shared by either branch (for example commandElems). Once the RedisResponse is done we can then fall through to a command return.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5544:
URL: https://github.com/apache/geode/pull/5544#discussion_r494535974
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/SubscriptionsIntegrationTest.java
##########
@@ -39,9 +38,8 @@
public static ExecutorServiceRule executor = new ExecutorServiceRule();
@Test
- @Ignore("GEODE-8515")
public void pingWhileSubscribed() {
- Jedis client = new Jedis("localhost", server.getPort());
+ Jedis client = new Jedis("localhost", server.getPort(), 1000000000);
Review comment:
Thank you for calling this out. I had a long timeout for debugging and forgot to remove it.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/PingExecutor.java
##########
@@ -31,9 +32,17 @@ public RedisResponse executeCommand(Command command,
ExecutionHandlerContext context) {
List<byte[]> commandElems = command.getProcessedCommand();
byte[] result = PING_RESPONSE.getBytes();
+ byte[] subscribeResult = "".getBytes();
if (commandElems.size() > 1) {
result = commandElems.get(1);
+ subscribeResult = result;
}
+
+ if (!context.getPubSub().findSubscribedChannels(context.getClient()).isEmpty()) {
Review comment:
I refactored it. Check it out and let me know what you think.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #5544:
URL: https://github.com/apache/geode/pull/5544
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5544:
URL: https://github.com/apache/geode/pull/5544#discussion_r494535974
##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/SubscriptionsIntegrationTest.java
##########
@@ -39,9 +38,8 @@
public static ExecutorServiceRule executor = new ExecutorServiceRule();
@Test
- @Ignore("GEODE-8515")
public void pingWhileSubscribed() {
- Jedis client = new Jedis("localhost", server.getPort());
+ Jedis client = new Jedis("localhost", server.getPort(), 1000000000);
Review comment:
Thank you for calling this out. I had a long timeout for debugging and forgot to remove 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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on pull request #5544:
URL: https://github.com/apache/geode/pull/5544#issuecomment-698047025
Distributed test failure is unrelated to this PR.
----------------------------------------------------------------
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:
users@infra.apache.org