Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ping connecion handler #2253

Conversation

yangbodong22011
Copy link
Contributor

@yangbodong22011 yangbodong22011 commented Nov 17, 2022

PingConnectionHandler will periodically send PING commands to Redis Server, and decide whether to reconnect based on whether it fails (the current strategy is to reconnect after three consecutive failures)

This is essentially because KeepAlive that only relies on TCP is unreliable, for details, please refer to: #2082

change

  • Add PingConnectionHandler, and decide whether to enable it according to the size of pingConnectionInterval (default is 0, not enabled,unit: ms).

  • If the sendPing command fails 3 times in a row (defined by MAX_PING_FAILED_TIMES and not user-configurable), it will trigger channel.close and then reconnect.

  • The return result of the PING command ignores RedisLoadingException and RedisBusyException exceptions.

test

There is no good way to unit test, so I manually verified it using iptables as mentioned in the original issue and the test passed.

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

PingConnectionHandler will periodically send PING commands to Redis Server, and decide whether to reconnect based on whether it fails (the current strategy is to reconnect after three consecutive failures)

This is essentially because KeepAlive that only relies on TCP is unreliable, for details, please refer to: redis#2082
@mp911de
Copy link
Collaborator

mp911de commented Nov 17, 2022

Thanks for reaching out. That isn't something we're interested in as we have better measures (keep-alive) that doesn't interfere with the Redis protocol.

@mp911de mp911de closed this Nov 17, 2022
@mp911de mp911de added the status: declined A suggestion or change that we dont feel we should currently apply label Nov 17, 2022
@shelltea
Copy link

shelltea commented Nov 18, 2022

@mp911de Hi, There are lots of issues about this problem.
#1466
#1428
#1727
Since you have closed this PR, is there a more reasonable way for us to resolve this issue?

@yangbodong22011
Copy link
Contributor Author

Update:

  1. By adding BlockingCommands to identify blocking commands and avoid sending PING.
  2. For the Module Blocking command, users can customize it through io.lettuce.core.BlockingCommands#addBlockingCommand.
  3. Pass COMMANDS_STACK through Channel.attr.

@mp911de pls have a look when you have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we dont feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants