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

Fix role check in configuration example #7

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

jacobvosmaer
Copy link
Contributor

The documentation suggested that when using Redis Sentinel, we should call sentinel.TestRole() every time we retrieve a Redis connection from the connection pool. This runs against the Redis documentation, which says that Sentinel clients should check the server role just once after they established a connection.
https://redis.io/docs/reference/sentinel-clients/

If users of sentinel-go follow the old configuration example they will make many unnecessary ROLE calls to Redis.

This commit fixes the documentation to suggest that users call sentinel.TestRole() from within the Dial callback. This matches what the Redis documentation suggests we do.

The documentation suggested that when using Redis Sentinel, we should
call sentinel.TestRole() every time we retrieve a Redis connection
from the connection pool. This runs against the Redis documentation,
which says that Sentinel clients should check the server role just
once after they established a connection.
https://redis.io/docs/reference/sentinel-clients/

If users of sentinel-go follow the old configuration example they will
make many unnecessary ROLE calls to Redis.

This commit fixes the documentation to suggest that users call
sentinel.TestRole() from within the Dial callback. This matches what
the Redis documentation suggests we do.
@FZambia
Copy link
Owner

FZambia commented Sep 26, 2022

Thanks, so according to this:

Starting with Redis 2.8.12, when Redis Sentinel changes the configuration of an instance, for example promoting a replica to a master, demoting a master to replicate to the new master after a failover, or simply changing the master address of a stale replica instance, it sends a CLIENT KILL type normal command to the instance in order to make sure all the clients are disconnected from the reconfigured instance. This will force clients to resolve the master address again.

– connections in the pool should be closed automatically and this triggers a new Dial. Do I understand it right?

@jacobvosmaer
Copy link
Contributor Author

Yes, that is correct.

As far as I understand the purpose of the ROLE check is to make sure there was no failover between the moment we looked up the master and the moment we connected to the master.

@FZambia FZambia merged commit decafca into FZambia:master Sep 26, 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.

2 participants