Conversation
twexler
added
do not merge
|
Thanks @arschles! Right now, I'm actually in progress of upgrading the redis module to v7 to support authenticated sentinels, this PR may expand in scope a bit. I'll keep it updated! |
|
|
||
| // WithRedisSentinelLock returns a distributed singleflight | ||
| // with a redis cluster that utilizes sentinel for quorum and failover | ||
| func WithRedisSentinelLock(endpoints []string, master, password string, checker storage.Checker) (Wrapper, error) { |
There was a problem hiding this comment.
This could alternatively be compacted into WithRedisLock, but I didn't want to add too much complexity. Thoughts?
There was a problem hiding this comment.
I like that you've separated sentinel from redis itself. sentinel is treated differently enough in the redis docs that I think we should treat it differently here from a docs and code perspective.
marwan-at-work
left a comment
marwan-at-work
left a comment
There was a problem hiding this comment.
Quick review :) Thanks for the library upgrades 💯
|
|
||
| // WithRedisSentinelLock returns a distributed singleflight | ||
| // with a redis cluster that utilizes sentinel for quorum and failover | ||
| func WithRedisSentinelLock(endpoints []string, master, password string, checker storage.Checker) (Wrapper, error) { |
There was a problem hiding this comment.
I like that you've separated sentinel from redis itself. sentinel is treated differently enough in the redis docs that I think we should treat it differently here from a docs and code perspective.
|
@twexler @marwan-at-work I think we should decide if we want to make sentinel support configurable as part of the existing redis config, or a new config section in the TOML. I thought it over and I vote for a separate config (even though it's the same client in the code). It's more work for us on almost all fronts, but I believe it'll be harder to write docs, but easier to document. And I think a separate documentation section for sentinel vs. non-sentinel will be easier to understand for users. Can you 👍 on this if you share my vote (I've already cast my vote here), and 👎 on this if you want to merge the configs? |
|
I'm okay with keeping it separate, but in that case...let me take the docs question on in this PR as well, so we can have full agreement on how to document this |
|
sounds good @twexler |
chriscoffee
left a comment
chriscoffee
left a comment
There was a problem hiding this comment.
Looks good to me just some typos!
arschles
left a comment
arschles
left a comment
There was a problem hiding this comment.
🎉 🚀
Thanks again for this @twexler !
Since #1554 (comment) has 2 votes, and there were 3 people in that particular discussion about combining the sentinel config to the single-node config, vs. separate configs (what this PR has currently), we are going to go with the config situation in this PR as-is.
4 participants
What is the problem I am trying to address?
Redis sentinel support
How is the fix applied?
Adds a
WithRedisSentinelLocktostashpackage, adds various configuration bits needed.Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.
Fixes #1553