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 the :nearest_slave role for Sentinel mode #588

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cheald
Copy link
Contributor

@cheald cheald commented Feb 27, 2016

This will cause the Sentinel client, upon connection, to measure roundtrip latency to each slave and select the slave with the lowest latency. The intent for this is to enable sentinel-managed clusters of servers for which eventually-consistent reads are acceptable, but to maintain minimum latencies between any individual client-slave pair.

The case I did this for is is shared web application caching across multiple datacenters, where you would not want Redis to read from a slave in another datacenter, but you want to be able to share the cache across the datacenter.

(If this would be more appropriate as a separate gem or something, I'm happy to do that, but there didn't seem to be enough exposed API to accomplish it without a PR.)

@cheald
Copy link
Contributor Author

cheald commented Mar 1, 2016

The 1.8.7 failures appear to be present in master, as well, running them locally with BUNDLE_GEMFILE=$PWD/.travis/Gemfile rake. Running just rake passes the suite as expected. I'm reasonably confident that these changes haven't caused that break.

@cheald
Copy link
Contributor Author

cheald commented Mar 1, 2016

It looks like the issue is the difference between test_unit 3.1.5 and 3.1.7. Locking test_unit to 3.1.5 causes the suite to pass.

@badboy
Copy link
Contributor

badboy commented Mar 3, 2016

Would you mind submitting the version pinning separately? Then we can already merge that regardless of whether this new feature is accepted.

@cheald
Copy link
Contributor Author

cheald commented Mar 4, 2016

Certainly! You'll find it at #593

@cheald cheald force-pushed the nearest-slave branch 2 times, most recently from 998fb73 to 14b86ef Compare April 21, 2016 08:09
@byroot byroot force-pushed the master branch 6 times, most recently from efe158b to dd4b6fe Compare June 9, 2020 11:23
@ggmichaelgo
Copy link

@cheald Can you update your PR please?
This PR would be amazing to be merged....

One of our redis replica is in US Eastern, and one of our Rails instance in US Western region...

The delay between two instances are 60ms~, but ideally our Rails application should only connect to its region's redis

cheald added 2 commits August 17, 2020 10:05
This will cause the client to measure roundtrip latency to each slave
and select the slave with the lowest latency. The intent for this is
to enable sentinel-managed clusters of servers for which eventually-consistent
reads are acceptable, but to maintain minimum latencies between any
individual client-slave pair.

The case I did this for is is shared web application caching across multiple
datacenters, where you would not want Redis to connect to a slave in another
datacenter, but you would want all datacenters to share a cache.

Remove trailing comma from client creation options; should fix 1.8 builds

If we can't get the role, use a translated role

Ensure that ping test clients are always disconnected after use. Don't assume that a good slave was found.
@cheald
Copy link
Contributor Author

cheald commented Aug 17, 2020

Looks like a simple merge. I've rebased it to master. I don't currently have a production use case for this, but let me know if it works for you.

@byroot
Copy link
Collaborator

byroot commented Aug 17, 2020

Rubocop has a few complaints.

The code looks sane to me, but @supercaracal if you have time and are willing to give this a second look, it would be much appreciated.

@cheald
Copy link
Contributor Author

cheald commented Aug 17, 2020

I'll see what I can do to make Rubocop happy.

Copy link
Contributor

@supercaracal supercaracal left a comment

Choose a reason for hiding this comment

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

CI is failed but these test cases are not related the PR. We can ignore them. #930

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.

5 participants