Redis client connection/reconnection fix #439
Conversation
| connectTimeout: OptArg[FiniteDuration] = OptArg.Empty, | ||
| idleTimeout: OptArg[FiniteDuration] = OptArg.Empty, | ||
| maxWriteSizeHint: OptArg[Int] = 50000, | ||
| initTimeout: FiniteDuration = 5.seconds, |
There was a problem hiding this comment.
How about initResponseTimeout, it would explain this setting better
Also, I think 5 seconds default value is too strict for a library, I would vote for at least 15.
| waitUntil( | ||
| for { | ||
| stateOk <- c.clusterInfo.map(_.stateOk) | ||
| slavesHaveSlots <- c.clusterSlots.map(_.forall(_.slaves.nonEmpty)) |
There was a problem hiding this comment.
In case of failure tests we were performing shutdown commands (and maybe other things) before cluster was properly setup
| * (especially Connected message if arrived after ConnectionClosed because of some race conditions). | ||
| * Without this state, we would process old ones instead of dropping them and creating a new connection. | ||
| */ | ||
| private def reconnecting(retryStrategy: RetryStrategy): Receive = { |
There was a problem hiding this comment.
I guess this behaviour should also react to other steering messages like Open, IncomingPacks, Release or Close
There was a problem hiding this comment.
My intention was to do not react to those as if we reconnecting, it means we already closed/lost connection so all old messages can be dropped. Handling those messages would cause reacting to messages from connection that does not longer exist
There was a problem hiding this comment.
My concern is based on the fact that every other behaviour at least supports the IncomingPacks case. We cannot drop these messages because of the nature of RedisConnectionActor. Look at RedisConnectionClient, its primary duty is to ask the connectionActor using the appropriate Redis command and wait for the response. This client sends asks to the actor whenever it's ready and considers itself ready until it gets explicitly closed.
Your approach of not reacting to messages until the actor reconnects successfully is reasonable but it doesn't take into account the async nature of the communication between client and connection actor. For the client, no response means the request is still being processed which in this case is not particularly true. Connection actor should either:
- Queue the request and try to perform it after reconnecting (this is what the behaviour was before changes from this PR)
- Respond with an error
| case Connected(connection, remoteAddress, localAddress) => | ||
| log.debug(s"Connected to Redis at $address") | ||
| //TODO: use dedicated retry strategy for initialization instead of reconnection strategy | ||
| new ConnectedTo(connection, localAddress, remoteAddress).initialize(config.reconnectionStrategy) |
There was a problem hiding this comment.
I think we should also apply connectionId to Connected, we shouldn't go into initialized if Connected comes from different connection
| log.error(s"Received Connected for connection different than currently trying to establish") | ||
| connection ! CloseConnection(immediate = true) | ||
| } else { | ||
| //TODO: use dedicated retry strategy for initialization instead of reconnection strategy |
There was a problem hiding this comment.
You miss log.debug(s"Connected to Redis at $address"), I guess it's an accident
3 participants
If PR fixes few issues: