[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5349: GEODE-8333: Fix PUBSUB hang

[GitBox <gi...@apache.org>]

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



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -191,38 +199,121 @@ public void channelInactive(ChannelHandlerContext ctx) {
     ctx.close();
   }
 
-  private void executeCommand(ChannelHandlerContext ctx, Command command) {
-    RedisResponse response;
-
-    if (!isAuthenticated()) {
-      response = handleUnAuthenticatedCommand(command);
-      writeToChannel(response);
-      return;
+  private void startAsyncCommandExecution(Command command) {
+    if (logger.isDebugEnabled()) {
+      logger.debug("Starting execution of async Redis command: {}", command);
     }
+    final long start = redisStats.startCommand(command.getCommandType());
+    command.setAsyncStartTime(start);
+    command.execute(this);
+  }
 
-    if (command.isUnsupported() && !allowUnsupportedCommands()) {
-      writeToChannel(
-          RedisResponse.error(command.getCommandType() + RedisConstants.ERROR_UNSUPPORTED_COMMAND));
-      return;
+  public void endAsyncCommandExecution(Command command, ByteBuf response) {
+    synchronized (commandQueue) {
+      Command head = takeFromCommandQueue();
+      if (head != command) {
+        throw new IllegalStateException(
+            "expected " + command + " but found " + head + " in the queue");

Review comment:
       No. This is basically a check to make sure the implementation is correct. It should never happen if it is.
   So the only way I could think of getting it to happen under test would be to have a unit test that mocks takeFromCommandQueue to return a bogus command. But I don't see much value in adding that test.




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