dschneider-pivotal commented on a change in pull request #5488:
URL: https://github.com/apache/geode/pull/5488#discussion_r480229528
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -61,17 +64,22 @@
public class ExecutionHandlerContext extends ChannelInboundHandlerAdapter {
private static final Logger logger = LogService.getLogger();
+ private static final Command TERMINATE_COMMAND = new Command();
private final Client client;
private final Channel channel;
private final RegionProvider regionProvider;
private final PubSub pubsub;
- private final EventLoopGroup subscriberGroup;
private final ByteBufAllocator byteBufAllocator;
private final byte[] authPassword;
private final Supplier<Boolean> allowUnsupportedSupplier;
private final Runnable shutdownInvoker;
private final RedisStats redisStats;
+ private final EventLoopGroup subscriberGroup;
+ private final int MAX_QUEUED_COMMANDS =
+ Integer.getInteger("geode.redis.commandQueueSize", 1000);
+ private final LinkedBlockingQueue<Command> commandQueue =
+ new LinkedBlockingQueue<>(100);
Review comment:
Instead of "100" should this be "MAX_QUEUED_COMMANDS"?
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -247,25 +306,16 @@ private RedisResponse
handleUnAuthenticatedCommand(Command command) {
return response;
}
- public EventLoopGroup getSubscriberGroup() {
- return subscriberGroup;
- }
-
- public void changeChannelEventLoopGroup(EventLoopGroup newGroup) {
- if (newGroup.equals(channel.eventLoop())) {
- // already registered with newGroup
- return;
- }
- channel.deregister().addListener((ChannelFutureListener) future -> {
- newGroup.register(channel).sync();
- });
- }
-
- private void logResponse(RedisResponse response) {
+ private void logResponse(RedisResponse response, String extraMessage,
Throwable cause) {
if (logger.isDebugEnabled() && response != null) {
ByteBuf buf = response.encode(new UnpooledByteBufAllocator(false));
- logger.debug("Redis command returned: {}",
- Command.getHexEncodedString(buf.array(), buf.readableBytes()));
+ if (cause == null) {
+ logger.debug("Redis command returned: {} - {}",
+ Command.getHexEncodedString(buf.array(), buf.readableBytes()),
extraMessage);
+ } else {
+ logger.warn("Redis command FAILED to return: {} - {}",
Review comment:
Does this warning message make sense? It might but I thought it would
just say it FAILED I'm not sure about "to return"
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -192,43 +244,50 @@ public void channelInactive(ChannelHandlerContext ctx) {
if (logger.isDebugEnabled()) {
logger.debug("GeodeRedisServer-Connection closing with " +
ctx.channel().remoteAddress());
}
+ commandQueue.offer(TERMINATE_COMMAND);
Review comment:
I think we should review the commandQueue code and see if we might
continue to process commands in the queue after this happens. For example this
code adds TERMINATE_COMMAND to the end of the queue so do we end up still
processing any commands already queued? We should also check that the "quit"
command does not allow commands queued after it to still be processed on the
server
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -139,6 +164,33 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) {
}
}
+ public EventLoopGroup getSubscriberGroup() {
+ return subscriberGroup;
+ }
+
+ public synchronized void changeChannelEventLoopGroup(EventLoopGroup newGroup,
Review comment:
I'd like to discuss what this should synchronize on. The method syncs on
this (ExecutionHandlerContext) but the future it creates syncs on the channel.
I think it would be better to sync on "channel" on this method instead of
"this"
----------------------------------------------------------------
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
