-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
@ssl = !!master_options&.key?(:ssl_context) | ||
@scheme = "redis#{@ssl ? 's' : ''}" | ||
Comment on lines
+31
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = {} | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -116,7 +119,6 @@ def resolve_slave | |
protected | ||
|
||
def assign_default_tags(tags) | ||
tags[:protocol] = @protocol.to_s | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
There was a problem hiding this comment.
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.