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

Add Redis connection health checks and restarts #55

Merged

Conversation

mjeffrey18
Copy link
Contributor

@mjeffrey18 mjeffrey18 commented Oct 8, 2022

Redis has complexities that need to be considered;

  1. Redis Pub/Sub works well until you lose the connection...
  2. Redis connections can go stale without activity.
  3. Redis DB's have a buffer related to the message sizes called Output Buffer Limits. Exceeding this buffer will not disconnect the connection. It just yields it dead. You cannot know about this except by monitoring logs/metrics.

This PR helps to mitigate issues 1 and 2

We've been running these changes in production (with these changes #54 and a different Redis shard) with excellent results;

1 month of APM metrics

Screenshot 2022-10-08 at 18 50 23

NOTE: No automated restarts at all in the last month

Workloads
Screenshot 2022-10-08 at 18 50 47
Screenshot 2022-10-08 at 18 30 45

NOTE: Those errors are all just from Lucky No route (spam requests)

2 Servers will low resource usage

Screenshot 2022-10-08 at 18 29 18

@mjeffrey18
Copy link
Contributor Author

@jwoertink is ready for initial impressions review. I still have to add specs. I will sort that out tomorrow.
I've changed some of the logging to match your PR #48

@jwoertink
Copy link
Collaborator

Thanks @mjeffrey18! @russ and @franciscoGPS if either of you have time, I'd love more eyes on this.

@mjeffrey18 mjeffrey18 force-pushed the handle-redis-connection-issues branch from a4b7828 to b6994f0 Compare October 8, 2022 18:45
@russ
Copy link
Contributor

russ commented Oct 9, 2022

This looks awesome. Can’t wait to try it out.

@mjeffrey18
Copy link
Contributor Author

This PR plus - jgaskins/redis#35 is what we are running in production

@mjeffrey18 mjeffrey18 force-pushed the handle-redis-connection-issues branch from b6994f0 to 5150649 Compare October 16, 2022 09:48
@mjeffrey18 mjeffrey18 changed the title [WIP] Add Redis connection health checks and restarts Add Redis connection health checks and restarts Oct 16, 2022
@mjeffrey18
Copy link
Contributor Author

@jwoertink Added specs for this also. We can merge if you guys are ready.

@@ -197,6 +199,64 @@ class ChatChannel < ApplicationCable::Channel
end
```

## Redis
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this section on README is amazing, thanks for such effort 😎 👍

Copy link
Collaborator

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet! Thanks for doing this. Sorry for the delay, I was at Euruko last week. Just got back and catching up.

@jwoertink jwoertink merged commit 7418fef into cable-cr:master Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants