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:
us...@infra.apache.org
