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 host_to_name_map option to Dalli::Protocol::ConnectionManager #1005

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

philister
Copy link

@philister philister commented Jun 13, 2024

New config-parameter: host_to_name_map
a hash mapping "host:port" to a name. Useful for providing more descriptive names or tuning the ring-continuum
e.g.
client1 { 'localhost:11211' => 'memcached1', 'mem2.cach.ed' => 'memcached2'}
client2 { 'localhost:11211' => 'memcached2', 'mem1.cach.ed' => 'memcached1'}

For a consistent calculation of which key can be found on which server, the config must also be the same on all clients. Previously, this was only possible if all memcached servers were accessible at the same address on all clients.
However, you may want or be able to reach the memcache servers from different clients only via different addresses (local vs public vs docker vs dns, etc)

@philister philister force-pushed the custom_server_name branch from 25d9bf0 to 33ba022 Compare July 8, 2024 08:31
Copy link
Collaborator

@grcooper grcooper left a comment

Choose a reason for hiding this comment

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

This name method is buried in the library behind a collection of private methods. Ie to fetch it I believe you have to do things like use the send method? So I am curious as to what you want to do with "tuning the ring-continuum", as that is locked inside of Dalli itself generally.

c = Dalli::Client.new

c.send(:ring).servers.name # needing to do send is a code smell

I also know it's used in stats and version as well, but if you are returning this data in your app I assume you have some kind of name to server mapping that already exists and could map the results as needed?

I just don't want to add more complexity to the top-level options if unnecessary.

@@ -45,7 +47,13 @@ def name
if socket_type == :unix
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we not also doing this for unix sockets?

"#{hostname}:#{port}"
name = "#{hostname}:#{port}"

if options[:host_to_name_map][name].nil?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be simplified to:

options[:host_to_name_map][name] || name

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