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 support for redis-sentinel #95

Merged
merged 5 commits into from
Feb 1, 2023
Merged

Add support for redis-sentinel #95

merged 5 commits into from
Feb 1, 2023

Conversation

acteru
Copy link
Contributor

@acteru acteru commented Jan 30, 2023

Purpose of proposed changes

Closes #94

  • Add Redis sentinel as caching backend
  • Add configuration example

Essential steps taken

  • Tested with a redis-sentinel three node test cluster configured on rhel 9.
  • It would be very good if someone else could validate this with their own setup so nothing is forgotten

@acteru
Copy link
Contributor Author

acteru commented Jan 31, 2023

Does anyone have a suggestion how it would be possible to test this automatically? I don't really have much experience with testing, but would be willing to work my way into it.

@Snawoot
Copy link
Owner

Snawoot commented Feb 1, 2023

Hello, @acteru!

Sorry for late response.

IIRC similar module for redis is tested by e2e tests with actual redis, so in this case we will need actual sentinel cluster in test env. It's doable, but quite laborious and not a priority because feature is quite isolated from other code and can be tested at least manually. Anyway, tests can be added later: feature with no tests is better than absence of feature.

About things that forgotten and possible corner cases. I'm not sure application will handle properly election of new master server by sentinel. Code stores connection pool for master once and very likely it will not be valid if master was switched to another server (e.g. if old server just become unavailable). Unfortunately, I don't know how to approach this properly: store once as it is already done, or evaluate before each interaction with redis or reevaluate it once in a while (regularly or using some caching decorator with small time to live). I haven't much of experience with sentinel and python redis driver. If possible please check how it behaves after master switch. I have a feeling it can be improved in follow up pull requests.

All in all, great job! Thank you for the contribution!

@Snawoot Snawoot merged commit f9033f6 into Snawoot:master Feb 1, 2023
@Snawoot
Copy link
Owner

Snawoot commented Feb 1, 2023

@acteru
+ if you don't mind, would be nice to update man page (man/mta-sts-daemon.yml.5.adoc) to advertise new kind of supported cache.

@acteru
Copy link
Contributor Author

acteru commented Feb 13, 2023

@Snawoot What I was able to find out so far is that the sentinel client supports a reconnect. I get an error that the current master cannot be found, but after the master selection everything works again. For testing I did a manual failover, as long as the master is not available I get the following error and the stacktrace 2023-02-13 10:46:22 ERROR STS: Cache set failed: No master found for 'mymaster' but the daemon keeps running, at least started over cli. Then after the failover i get: 2023-02-13 11:37:05 ERROR STS: Cache get failed: Connection closed by server. After checking with a few keys, everything still seems to be working correctly. Perhaps it would be necessary to handle the exceptions a bit nicer, but on the whole everything seems to be working

@Snawoot
Copy link
Owner

Snawoot commented Feb 13, 2023

@acteru I think it's already good enough. Will make a new release as soon as I can.

@Snawoot
Copy link
Owner

Snawoot commented Feb 13, 2023

Released in v1.2.0

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.

Support for redis sentinel
2 participants