upthewaterspout commented on a change in pull request #6805:
URL: https://github.com/apache/geode/pull/6805#discussion_r696780446
##########
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);
Review comment:
I think the ForceDisconnectException should be handled the same was as a
CacheClosedException. It's just another way that the local cache may have been
shutdown.
I think the best thing to do would be to change the above catch clause for
CacheClosedException to handle the parent class `CancelException`, which covers
ForceDisconnectException, CacheClosedException, and other shutdown related
exceptions.
BTW, this whole handle method looks scary to me! In general I don't think we
should be fishing for root causes of exceptions - that seems like maybe bad
exception handling in other layers if we have to do that.
--
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]
