Skip to content

Sentinels: Accept options to connect to master #61

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions lib/async/redis/sentinel_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@ class SentinelClient
#
# @property endpoints [Array(Endpoint)] The list of sentinel endpoints.
# @property master_name [String] The name of the master instance, defaults to 'mymaster'.
# @property master_options [Hash] Connection options for master.
# @property role [Symbol] The role of the instance that you want to connect to, either `:master` or `:slave`.
# @property protocol [Protocol] The protocol to use when connecting to the actual Redis server, defaults to {Protocol::RESP2}.
def initialize(endpoints, master_name: DEFAULT_MASTER_NAME, role: :master, protocol: Protocol::RESP2, **options)
def initialize(endpoints, master_name: DEFAULT_MASTER_NAME, master_options: nil, role: :master, **options)
@endpoints = endpoints
@master_name = master_name
@master_options = master_options
@role = role
@protocol = protocol
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the @protocol instance variable because, AFAIK, it is only used to connect to the master, and at this point we still don't know which one will be.


@ssl = !!master_options&.key?(:ssl_context)
@scheme = "redis#{@ssl ? 's' : ''}"
Comment on lines +31 to +32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't kinda like this, any ideas?


# A cache of sentinel connections.
@sentinels = {}
Expand Down Expand Up @@ -88,8 +91,8 @@ def resolve_master
rescue Errno::ECONNREFUSED
next
end
return Endpoint.remote(address[0], address[1]) if address

return Endpoint.for(@scheme, address[0], port: address[1], **@master_options) if address
end

return nil
Expand All @@ -107,7 +110,7 @@ def resolve_slave
next if slaves.empty?

slave = select_slave(slaves)
return Endpoint.remote(slave["ip"], slave["port"])
return Endpoint.for(@scheme, slave["ip"], port: slave["port"], **@master_options)
end

return nil
Expand All @@ -116,7 +119,6 @@ def resolve_slave
protected

def assign_default_tags(tags)
tags[:protocol] = @protocol.to_s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what is this for. I hope I'm not breaking anything.

end

# Override the parent method. The only difference is that this one needs to resolve the master/slave address.
Expand All @@ -127,8 +129,8 @@ def make_pool(**options)
endpoint = resolve_address
peer = endpoint.connect
stream = ::IO::Stream(peer)
@protocol.client(stream)

endpoint.protocol.client(stream)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we use the endpoint protocol to connect to the endpoint, I think this is more correct and removes the need for the instance variable.

end
end

Expand Down
Loading