Skip to content
This repository was archived by the owner on Sep 22, 2022. It is now read-only.
Open
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require 'master_slave_adapter'
68 changes: 61 additions & 7 deletions lib/master_slave_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ def master_slave_connection(config)
ConnectionAdapters::MasterSlaveAdapter.new(config, logger)
end

def mysql_master_slave_connection(config)
master_slave_connection(config)
end

private

def massage(config)
Expand All @@ -51,6 +55,7 @@ def massage(config)
reject { |k,_| skip.include?(k) }.
merge(:adapter => config.fetch(:connection_adapter))
([config.fetch(:master)] + config.fetch(:slaves, [])).map do |cfg|
cfg[:database] = defaults[:database] if defaults.has_key?(:database)
cfg.symbolize_keys!.reverse_merge!(defaults)
end
config
Expand All @@ -73,6 +78,21 @@ def load_adapter(adapter_name)
end
end
end

module MasterSlaveBehavior
def self.included(base)
base.alias_method_chain(:reload, :master_slave)
end

# Force reload to use the master connection since it's probably being called for a reason.
Copy link

Choose a reason for hiding this comment

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

I find this too dangerous, too, as a default behavior, as it essentially bypasses the assumptions the user may have about consistency guarantees. How about a reload_from_master method which would have to be called explicitly?

def reload_with_master_slave(*args)
self.class.with_master do
reload_without_master_slave(*args)
end
end
end

include(MasterSlaveBehavior) unless include?(MasterSlaveBehavior)
end

module ConnectionAdapters
Expand Down Expand Up @@ -121,11 +141,25 @@ def initialize(config, logger)

@connections = {}
@connections[:master] = connect(config.fetch(:master), :master)
@connections[:slaves] = config.fetch(:slaves).map { |cfg| connect(cfg, :slave) }

@connections[:slaves] = []
if config[:slaves]
config.fetch(:slaves).each do |cfg|
begin
@connections[:slaves] << connect(cfg, :slave)
rescue StandardError => e
Copy link

Choose a reason for hiding this comment

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

This doesn't seem like a good idea. What if you can't connect to /any/ slave? Your master is probably going to be busted then...
I think that if you want to be tolerant against slave availability, the logic needs to be way more sophisticated.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe. I just don't like the requirement for the site to be up at all to be spread across N servers. Before doing this, it's a single point of failure, but if you require all slaves to be on / reachable, then it's the number of slaves + 1 that bring the whole situation down. So overall, I say to just not worry about that. There are other notifications in place to know that happened, but the app should stay alive.

Rails.logger.error "Slave can not connect ::: #{cfg.inspect}"
end
end
end

@disable_connection_test = config.delete(:disable_connection_test) == 'true'

self.current_connection = slave_connection!
if config.delete(:initial_connection) == 'master'
Copy link

Choose a reason for hiding this comment

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

Can you elaborate on the use case for this?

Copy link
Author

Choose a reason for hiding this comment

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

I wanted more control of the default case. In my controllers, I set it to use with_consistency but if I didn't say that or with_slave or with_master, I preferred it to just keep using master. For example, delayed_job queue processing or using console or whatever.

Copy link

Choose a reason for hiding this comment

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

fair enough. somehow announcing this to the user (log statement?) would be nice, though

self.current_connection = master_connection
else
self.current_connection = slave_connection!
end
end

# MASTER SLAVE ADAPTER INTERFACE ========================================
Expand Down Expand Up @@ -214,7 +248,16 @@ def active?
end

def reconnect!
self.connections.each { |c| c.reconnect! }
@connections[:slaves].delete_if do |slave|
begin
slave.reconnect!
false
rescue StandardError => e
Rails.logger.error "Slave can not reconnect! ::: #{slave}"
true
Copy link

Choose a reason for hiding this comment

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

Same as with initial connect

Copy link
Author

Choose a reason for hiding this comment

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

Ditto, we ran tests to turn power off of a single out of a group of slaves and wanted the site to keep functioning and stop trying to talk that one until told otherwise.

end
end
@connections[:master].reconnect!
end

def disconnect!
Expand Down Expand Up @@ -269,6 +312,7 @@ def execute(*args)
:to => :master_connection }])
# no clear interface contract:
delegate :tables, # commented in SchemaStatements
:indexes, # migrations
Copy link

Choose a reason for hiding this comment

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

nice

:truncate_table, # monkeypatching database_cleaner gem
:primary_key, # is Base#primary_key meant to be the contract?
:to => :master_connection
Expand Down Expand Up @@ -313,15 +357,15 @@ def master_connection
# Returns a random slave connection
# Note: the method is not referentially transparent, hence the bang
def slave_connection!
@connections[:slaves].sample
@connections[:slaves].sample || master_connection
Copy link

Choose a reason for hiding this comment

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

not good -- risking thundering herd here!

Copy link
Author

Choose a reason for hiding this comment

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

same philosophy on these - all the slaves can go away and it should still work. I suppose that's necessarily a thundering herd. clearly the wrong move if you'd rather not serve anything than serve it all off master. that the case?

Copy link

Choose a reason for hiding this comment

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

right, if the slaves have gone away (or lag behind, which is essentially the same case), I'd find it safer to assume that the site is experiencing unusually high load, and serving all off master could likely bring the master down as well. That's why I wrote the mechanism should be more sophisticated -- maybe requiring a minimum number of slaves to be available.

Another interesting thought would be to introduce some resilience against a master outage, where it would still be possible to satisfy reads.

end

def connections
@connections.values.inject([]) { |m,c| m << c }.flatten.compact
end

def current_connection
connection_stack.first
connection_stack.first || master_connection
Copy link

Choose a reason for hiding this comment

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

risk of thundering herd again

end

def current_connection=(conn)
Expand All @@ -338,17 +382,27 @@ def current_clock=(clock)

def master_clock
conn = master_connection
out = nil
if status = conn.uncached { conn.select_one("SHOW MASTER STATUS") }
Clock.new(status['File'], status['Position'])
out = Clock.new(status['File'], status['Position'])
end
if Rails.env.production?
Copy link

Choose a reason for hiding this comment

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

I kind of like this patch (as "meaningful nil" is always bad, well nil in general). But do you think there's a way to circumvent the assumption of a particular Rails.env?

out ||= Clock.infinity
else
out ||= Clock.zero
end
out
end

def slave_clock(conn)
out = nil
if status = conn.uncached { conn.select_one("SHOW SLAVE STATUS") }
Clock.new(status['Relay_Master_Log_File'], status['Exec_Master_Log_Pos']).tap do |c|
out = Clock.new(status['Relay_Master_Log_File'], status['Exec_Master_Log_Pos']).tap do |c|
set_last_seen_slave_clock(conn, c)
end
end
out ||= Clock.zero
out
end

def slave_consistent?(conn, clock)
Expand Down