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

Subclass switching #136

Merged
merged 18 commits into from
Oct 26, 2017
Merged
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
6 changes: 2 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,13 @@ basically connections inherit configuration from the parent configuration file.
## Usage

Normally you have some models that live on a shared database, and you might need to query this data in order to know what shard to switch to.
All the models that live on the shared database must be marked as not\_sharded:
All the models that live on the sharded database must inherit from ActiveRecordShards::ShardedModel:

class Account < ActiveRecord::Base
not_sharded

has_many :projects
end

class Project < ActiveRecord::Base
class Project < ActiveRecordShards::ShardedModel
belongs_to :account
end

Expand Down
3 changes: 1 addition & 2 deletions lib/active_record_shards.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require 'active_record/base'
require 'active_record_shards/configuration_parser'
require 'active_record_shards/model'
require 'active_record_shards/sharded_model'
require 'active_record_shards/shard_selection'
require 'active_record_shards/connection_switcher'
require 'active_record_shards/association_collection_connection_selection'
Expand All @@ -24,5 +25,3 @@ def self.rails_env
ActiveRecord::Relation.include(ActiveRecordShards::DefaultSlavePatches::ActiveRelationPatches)
ActiveRecord::Associations::CollectionProxy.include(ActiveRecordShards::AssociationCollectionConnectionSelection)
ActiveRecord::Associations::Builder::HasAndBelongsToMany.include(ActiveRecordShards::DefaultSlavePatches::HasAndBelongsToManyBuilderExtension)

ActiveRecord::InternalMetadata.not_sharded
27 changes: 14 additions & 13 deletions lib/active_record_shards/connection_switcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@ def self.extended(base)
base.singleton_class.send(:alias_method, :table_exists?, :table_exists_with_default_shard?)
end

def default_shard=(new_default_shard)
ActiveRecordShards::ShardSelection.default_shard = new_default_shard
switch_connection(shard: new_default_shard)
end

def on_shard(shard)
old_options = current_shard_selection.options
switch_connection(shard: shard) if supports_sharding?
Expand Down Expand Up @@ -135,7 +130,7 @@ def on_slave?
end

def current_shard_selection
Thread.current[:shard_selection] ||= ShardSelection.new
Thread.current[:shard_selection] ||= ShardSelection.new(shard_names.first)
end

def current_shard_id
Expand All @@ -146,27 +141,33 @@ def shard_names
unless config = configurations[shard_env]
raise "Did not find #{shard_env} in configurations, did you forget to add it to your database config? (configurations: #{configurations.inspect})"
end
unless config.fetch(SHARD_NAMES_CONFIG_KEY, []).all? { |shard_name| shard_name.is_a?(Integer) }
unless config[SHARD_NAMES_CONFIG_KEY]
raise "No shards configured for #{shard_env}"
end
unless config[SHARD_NAMES_CONFIG_KEY].all? { |shard_name| shard_name.is_a?(Integer) }
raise "All shard names must be integers: #{config[SHARD_NAMES_CONFIG_KEY].inspect}."
end
config[SHARD_NAMES_CONFIG_KEY] || []
config[SHARD_NAMES_CONFIG_KEY]
end

private

def switch_connection(options)
if options.any?
if options.key?(:slave)
current_shard_selection.on_slave = options[:slave]
end

if options.key?(:shard)
unless configurations[shard_env]
unless config = configurations[shard_env]
raise "Did not find #{shard_env} in configurations, did you forget to add it to your database config? (configurations: #{configurations.inspect})"
end
unless config['shard_names'].include?(options[:shard])
raise "Did not find shard #{options[:shard]} in configurations"
end
current_shard_selection.shard = options[:shard]
end

if options.key?(:slave)
current_shard_selection.on_slave = options[:slave]
Copy link
Member

@bquorning bquorning Oct 25, 2017

Choose a reason for hiding this comment

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

Do/should we support master/slave switching without switching shards?

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 you're getting at?

Copy link
Member

Choose a reason for hiding this comment

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

Me neither. This is fine.

end

ensure_shard_connection
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/active_record_shards/default_slave_patches.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ def model.on_slave_by_default?
left_reflection.klass.on_slave_by_default?
end

# also transfer the sharded-ness of the left table to the join model
model.not_sharded unless model.left_reflection.klass.is_sharded?
Copy link
Member

Choose a reason for hiding this comment

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

Will this be a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure :|

Copy link
Contributor

Choose a reason for hiding this comment

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

seems super sketchy that we call not_sharded ... maybe this is a 1-off join model ... which means that it should now be model.sharded if foo instead ?

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test for this case?

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've tried adding a test - it's not exactly obvious where to put it though

model.extend(ActiveRecordShards::Ext::ShardedModel) if model.left_reflection.klass.is_sharded?

model
end
end
Expand Down
24 changes: 5 additions & 19 deletions lib/active_record_shards/model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,14 @@
module ActiveRecordShards
module Model
def not_sharded
if self != ActiveRecord::Base && self != base_class
raise "You should only call not_sharded on direct descendants of ActiveRecord::Base"
end
self.sharded = false
ActiveSupport::Deprecation.warn("Calling not_sharded is deprecated. "\
"Please ensure to still read from the "\
"account db slave after removing the "\
"call.")
end

def is_sharded? # rubocop:disable Naming/PredicateName
Copy link
Member

Choose a reason for hiding this comment

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

Should we seize the moment and rename .is_sharded? to .sharded??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope ;)

if self == ActiveRecord::Base
sharded != false && supports_sharding?
elsif self == base_class
if sharded.nil?
ActiveRecord::Base.is_sharded?
else
sharded != false
end
else
base_class.is_sharded?
end
false
end

def on_slave_by_default?
Expand Down Expand Up @@ -61,9 +51,5 @@ def self.extended(base)
base.send(:include, InstanceMethods)
base.after_initialize :initialize_shard_and_slave
end

private

attr_accessor :sharded
end
end
16 changes: 5 additions & 11 deletions lib/active_record_shards/shard_selection.rb
Original file line number Diff line number Diff line change
@@ -1,20 +1,14 @@
# frozen_string_literal: true
module ActiveRecordShards
class ShardSelection
NO_SHARD = :_no_shard
cattr_accessor :default_shard

def initialize
def initialize(shard)
@on_slave = false
@shard = nil
self.shard = shard
end

def shard
if @shard.nil? || @shard == NO_SHARD
nil
else
@shard || self.class.default_shard
end
raise "Missing shard information on connection" unless @shard
@shard
end

PRIMARY = "primary".freeze
Expand All @@ -40,7 +34,7 @@ def resolve_connection_name(sharded:, configurations:)
end

def shard=(new_shard)
@shard = (new_shard || NO_SHARD)
@shard = Integer(new_shard)
end

def on_slave?
Expand Down
15 changes: 15 additions & 0 deletions lib/active_record_shards/sharded_model.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module ActiveRecordShards
module Ext
module ShardedModel
def is_sharded? # rubocop:disable Naming/PredicateName
true
end
end
end

class ShardedModel < ActiveRecord::Base
self.abstract_class = true

extend ActiveRecordShards::Ext::ShardedModel
end
end
2 changes: 1 addition & 1 deletion lib/active_record_shards/sql_comments.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def execute(query, name = nil)
end

def self.enable
ActiveRecord::Base.on_shard(nil) { ActiveRecord::Base.connection.class.prepend(Methods) }
ActiveRecord::Base.connection.class.prepend(Methods)
end
end
end
Loading