ReconnectingConnection: Add heartbeat check. #264
Conversation
shachlanAmazon
force-pushed
the
heartbeat
branch
from
3777063
to
aa6e74d
Compare
|
@barshaul moved the heartbeat logic to ClientCMD. |
| }; | ||
| log_debug("ClientCMD", "performing heartbeat"); | ||
| if connection | ||
| .send_packed_command(&redis::cmd("PING")) |
There was a problem hiding this comment.
I think we need to run it with timeout
There was a problem hiding this comment.
and what should we do on a timeout?
There was a problem hiding this comment.
I would retry the PING for 2-3 times if a timeout occur, then if timeout still persist close the connection and try to reconnect.
There was a problem hiding this comment.
why retry the ping?
There was a problem hiding this comment.
I think that for a response timeout we can retry before we decide that the connection needs to be re-established
There was a problem hiding this comment.
why isn't one timeout indicative?
There was a problem hiding this comment.
because we might fell on a time where the node was busy. But it doesnt matter, we can do it without retrying
| tokio::time::sleep(super::HEARTBEAT_SLEEP_DURATION).await; | ||
| if reconnecting_connection.is_dropped() { | ||
| log_debug("ClientCMD", "heartbeat stopped after client was dropped"); | ||
| // Client was dropped, heartbeat can stop. |
There was a problem hiding this comment.
It doesn't have to be that the client was dropped, we can drop the connection for topology changes
There was a problem hiding this comment.
so, replace "client" with "connection"?
| }) | ||
| }); | ||
|
|
||
| let _new_server = receiver.await; |
There was a problem hiding this comment.
can we check before that the connection is indeed in 'reconnecting' state?
There was a problem hiding this comment.
What does it mean that the CMD client is in reconnecting state? It will contain multiple internal connections.
There was a problem hiding this comment.
so we can check it throws a connection error
There was a problem hiding this comment.
this tests the client, not the connection. what does it mean that a client is in a reconnecting state?
There was a problem hiding this comment.
that we expect it to raise a connection error
There was a problem hiding this comment.
then how will the test differ from test_report_disconnect_and_reconnect_after_temporary_disconnect?
|
ping @barshaul |
shachlanAmazon
force-pushed
the
heartbeat
branch
from
aa6e74d
to
c716cbb
Compare
No description provided.