dschneider-pivotal commented on a change in pull request #6805:
URL: https://github.com/apache/geode/pull/6805#discussion_r696784306
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -180,6 +181,12 @@ private RedisResponse
getExceptionResponse(ChannelHandlerContext ctx, Throwable
} else if (rootCause instanceof InterruptedException
|| rootCause instanceof CacheClosedException) {
return RedisResponse.error(RedisConstants.SERVER_ERROR_SHUTDOWN);
+ } else if (rootCause instanceof ForcedDisconnectException) {
+ // This indicates a member departed or got disconnected
+ logger.warn(
+ "Closing client connection because one of the servers doing this
operation departed.");
+ channelInactive(ctx);
+ return null;
Review comment:
what does returning null do? Will it signal the client to retry the
operation? It seems like return
RedisResponse.error(RedisConstants.SERVER_ERROR_SHUTDOWN); would be better
since the server was forced to disconnect from the cluster.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -180,6 +181,12 @@ private RedisResponse
getExceptionResponse(ChannelHandlerContext ctx, Throwable
} else if (rootCause instanceof InterruptedException
|| rootCause instanceof CacheClosedException) {
return RedisResponse.error(RedisConstants.SERVER_ERROR_SHUTDOWN);
+ } else if (rootCause instanceof ForcedDisconnectException) {
Review comment:
A server can always be forced out of the geode cluster due to network
partitioning so I think we will always need to handle this exception.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -180,6 +181,12 @@ private RedisResponse
getExceptionResponse(ChannelHandlerContext ctx, Throwable
} else if (rootCause instanceof InterruptedException
|| rootCause instanceof CacheClosedException) {
return RedisResponse.error(RedisConstants.SERVER_ERROR_SHUTDOWN);
+ } else if (rootCause instanceof ForcedDisconnectException) {
+ // This indicates a member departed or got disconnected
+ logger.warn(
+ "Closing client connection because one of the servers doing this
operation departed.");
Review comment:
I think the ForcedDisconnectException will now apply to the current
server (i.e. the one logging this warning) so I'm not sure we should say "one
of the servers". Also I think we should tack onto the log string " + rootCause"
so that whatever info was in the ForcedDisconnectException will be in this
message. Should we also describe the type of client connection being closed?
When this log message is seen it will not be obvious that it is redis related.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
