Rails 4 #5

lukkry · 2013-08-25T21:12:15Z

This PR adds support for Rails 4.

I try to highlight the biggest changes and describe how I approached them, it's likely going to be a long PR message, so bear with me ;)

ConnectionSpecification lives in a different namespace, so ActiveRecordShards::ConnectionSpecification was introduced.

# lib/active_record_shards.rb
if ActiveRecord::VERSION::STRING >= "4.0.0"
  ActiveRecordShards::ConnectionSpecification =  ActiveRecord::ConnectionAdapters::ConnectionSpecification
else
  ActiveRecordShards::ConnectionSpecification = ActiveRecord::Base::ConnectionSpecification
end

ConnectionHandler was slightly redesigned and klass.name is called more often now (it's a key in @class_to_pool => all the methods where it's used are currently overrided and klass.connection_pool_name is used instead). If we want to stick with the current approach we'll have to override many more methods from ConnectionHandler. Therefore, I created ActiveRecordDecorator to avoid it. This class decorates ActiveRecord::Base object and returns connection pool name instead of its name. It allows us to override ConnectionHandler methods without changing or copying any logic:

# lib/active_record_shards/connection_pool.rb
def establish_connection_with_connection_pool_name(owner, spec)
  owner = ActiveRecordDecorator.new(owner)
  establish_connection_without_connection_pool_name(owner, spec)
end
alias_method_chain :establish_connection, :connection_pool_name

with_scope is depracated => scoping is used instead

# lib/active_record_shards/connection_switcher.rb
if ActiveRecord::VERSION::MAJOR == 2
  with_scope({:find => {:readonly => true}}, &block)
else
  readonly.scoping(&block)
end

ConnectionHandler#connection_pools is depracated, it is fairly simple, so I've just copy-pasted it

# lib/active_record_shards/connection_switcher.rb
def connected_to_shard?
  if ActiveRecord::VERSION::MAJOR == 4
    specs_to_pools = Hash[connection_handler.connection_pool_list.map { |pool| [pool.spec, pool] }]
  else
    specs_to_pools = connection_handler.connection_pools
  end

  specs_to_pools.has_key?(connection_pool_key)
end

Let me know what you think, appreciate any feedback! ;)

lukkry · 2013-08-25T21:15:57Z

cc @eac @staugaard

staugaard · 2013-08-27T16:46:25Z

@osheroff

osheroff · 2013-08-27T17:22:45Z

lib/active_record_shards/active_record_decorator.rb

+  #
+  class ActiveRecordDecorator < SimpleDelegator
+    def name
+      if __getobj__.respond_to? :connection_pool_name


this object is never used except to decorate ActiveRecord::Base, so it doesn't seem like this check is needed, yeah?

it's String when we call connection_handler.establish_connection method with explicit connection_pool_name, e.g. here:
https://github.com/zendesk/active_record_shards/blob/master/lib/active_record_shards/connection_switcher.rb#L185

In other words:

if __getobj__.is_a? String # assume it's connection_pool_name already __getobj__ else # assume it's ActiveRecord::Base object __getobj__.connection_pool_name end

I guess, ActiveRecordDecorator is a poor name and documentation is not 100% accurate, I'll change it ;)

ActiveRecordDecorator object should quack only with name method, so sth like OpenStruct might be another possible option

if args[0].respond_to? :connection_pool_name args[0] = OpenStruct.new(:name => args[0].connection_pool_name) elsif args[0].is_a? String args[0] = OpenStruct.new(:name => args[0]) end

it relies on fact, that ConnectionHandler calls only name method on klass|owner

This is how it may look like with OpenStruct: https://gist.github.com/lukkry/6370855
We can remove ActiveRecordDecorator entirely then.

I think I prefer the OpenStruct approach. It's more clear that "hey, we're just doing this to fool some methods we don't want to override".

osheroff · 2013-08-27T17:50:55Z

travis needs to be configured to actually build the appraisal

osheroff · 2013-08-27T17:51:34Z

Overall these changes look good, although I will say I found the decorator stuff to be pretty hard to read at first.

lukkry · 2013-08-28T19:40:46Z

Agreed, it's not super obvious. Actually, I've started with 2 separate files, each for a different rails version: connection_pool_rails3.rb with 2 overridden methods and connection_pool_rails4.rb with 5 overridden methods, and then I moved to Decorator .

imo it's not perfect but still better than maintaining different files for different rails version and overriding lots of AR internals

bquorning · 2013-08-29T21:03:46Z

lib/active_record_shards/connection_pool.rb

+                   else
+                     args[0].connection_pool_name
+                   end
+            args[0] = OpenStruct.new(:name => name)


Since OpenStruct has some performance issues (http://ruby-doc.org/, http://jamesgolick.com/), I must ask: could this be done with a Struct instead, i.e.

args[0] = Struct.new(:name).new(name)

didn't know that - thanks! ;) https://github.com/lukkry/active_record_shards/commit/500a6b9a922dd519c4f057503095b54036a3e75a

lukkry · 2013-08-29T21:06:11Z

I've just found out that some of the methods like: all, scoped, unscoped dont work on sharded DB (only in Rails 4). Probably, sth similar to this #4. I'll add more tests around it.

lukkry · 2013-09-03T20:28:07Z

Above errors are not Rails 4 related, See: #6

lukkry · 2013-09-07T20:23:10Z

lib/active_record_shards/connection_pool.rb

-  alias_method_chain :establish_connection, :connection_pool_name
-end
+module ActiveRecordShards
+  ConnectionPoolNameDecorator = Struct.new(:name)


I found a place where superclass method is called on this object: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L576
but imo we don't expect this code to be executed, pool_for(klass) always returns a pool. Otherwise we probably should go back to SimpleDelegator

Can someone confirm it?

I think superclass will be called. Can we just inject this into the Struct definition too?

ping @lukkry

@osheroff, I'm not sure if I get what do you want to inject?

I think that superclass is never called because pool_for method ultimately fetches data from @owner_to_pool variable and if our connection pool names are not there, there is nothing what we can do about it but raise ConnectionNotEstablished. It looks like Rails 4 assumes that there is always a key for ActiveRecord::Base in @owner_to_pool

Loading development environment (Rails 4.0.0) 2.0.0-p247 :001 > ActiveRecord::Base.connection_handler.send(:owner_to_pool).keys => ["ActiveRecord::Base"] 2.0.0-p247 :002 > Account.all Account Load (0.1ms) SELECT "accounts".* FROM "accounts" => #<ActiveRecord::Relation []> 2.0.0-p247 :003 > ActiveRecord::Base.connection_handler.remove_connection(ActiveRecord::Base) => {:adapter=>"sqlite3", :database=>"/Users/lukkry/projects/mango_test/db/development.sqlite3", :pool=>5, :timeout=>5000} 2.0.0-p247 :004 > Account.all ActiveRecord::ConnectionNotEstablished: ActiveRecord::ConnectionNotEstablished from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:546:in `retrieve_connection' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/connection_handling.rb:79:in `retrieve_connection' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/connection_handling.rb:53:in `connection' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/querying.rb:36:in `find_by_sql' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/relation.rb:585:in `exec_queries' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/relation.rb:471:in `load' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/relation.rb:220:in `to_a' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/activerecord-4.0.0/lib/active_record/relation.rb:573:in `inspect' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/railties-4.0.0/lib/rails/commands/console.rb:90:in `start' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/railties-4.0.0/lib/rails/commands/console.rb:9:in `start' from /Users/lukkry/.rvm/gems/ruby-2.0.0-p247/gems/railties-4.0.0/lib/rails/commands.rb:64:in `<top (required)>' from bin/rails:4:in `require' from bin/rails:4:in `<main>'

so it should be safe to assume that there is always a key for our connection pool name.

I run tests on master and this line is never executed.

re: superclass/rails3 yeah I guess so ;>
it wraps self.class into ConnectionPoolNameDecorator all the time and finally fails with:

<"undefined method `connection_pool_name' for ActiveRecordShards::ConnectionPoolNameDecorator:Class">

Don't have any reasonable solution yet.

I'm using this test (it's probably good only for rails 3 & 4):

it "raises an exception if a connection is not found" do ActiveRecord::Base.on_shard(0) do ActiveRecord::Base.connection_handler.remove_connection(Ticket) assert_raises(ActiveRecord::ConnectionNotEstablished) do ActiveRecord::Base.connection_handler.retrieve_connection_pool(Ticket) assert_using_database('ars_test_shard0', Ticket) end end end

one possible solution is to override retrive_connection_pool methods, but this is sth what I want to avoid from the beginning ;)

@osheroff I don't see what role sockets play here. As far as I understand pool_from_any_process_for, it uses only copied memory from a parent process. Is it how you see it or I'm missing sth obvious here?

In unix, when a child is forked off from the parent, it inherits its
parent's file descriptors (and thus sockets), and it's very bad to
re-use these in the child process. The rails code is probably ensuring
that children don't re-use their parents' sockets, and I wanted to make
sure that it works as expected.

On Mon, Sep 30, 2013 at 02:07:44PM -0700, Lukasz Krystkowiak wrote:

end
-end

-ActiveRecord::Base.singleton_class.class_eval do

def establish_connection_with_connection_pool_name(spec = nil)

case spec

when ActiveRecord::Base::ConnectionSpecification

connection_handler.establish_connection(connection_pool_name, spec)

else

establish_connection_without_connection_pool_name(spec)

end

end

alias_method_chain :establish_connection, :connection_pool_name
-end
+module ActiveRecordShards

ConnectionPoolNameDecorator = Struct.new(:name)

@osheroff I don't see what role sockets play here. As far as I understand pool_from_any_process_for, it uses only copied memory from a parent process. Is it how you see it or I'm missing sth obvious here?

Reply to this email directly or view it on GitHub:
https://github.com/zendesk/active_record_shards/pull/5/files#r6671278

re: superclass/rails3 I've overriden retrieve_connection_pool ;( and removed unneeded code (with superclass)

Before

# Rails 4 def retrieve_connection_pool(klass) class_to_pool[klass.name] ||= begin until pool = pool_for(klass) klass = klass.superclass break unless klass <= Base end class_to_pool[klass.name] = pool end end # Rails 3 & 2 def retrieve_connection_pool(klass) pool = @connection_pools[klass.name] return pool if pool return nil if ActiveRecord::Base == klass retrieve_connection_pool klass.superclass end

After

ActiveRecord::ConnectionAdapters::ConnectionHandler.class_eval do if ActiveRecord::VERSION::MAJOR >= 4 def retrieve_connection_pool(klass) class_to_pool[klass.connection_pool_name] ||= pool_for(klass) end else def retrieve_connection_pool(klass) (@class_to_pool || @connection_pools)[klass.connection_pool_name] end end end

lukkry · 2013-09-14T11:42:23Z

this is ready for re-review

bquorning · 2013-09-14T23:34:16Z

lib/active_record_shards/connection_pool.rb

+  # Rails version).
+  #
+  # It takes the first argument, ActiveRecord::Base object or
+  # String (connection_pool_name), converts it in OpenStruct


You can remove "Open" from that comment :-)

osheroff · 2013-11-05T19:02:22Z

hey @lukkry, I didn't see that you had added new commits. Think this is ready to merge (or at least pre-release)?

lukkry · 2013-11-05T21:58:54Z

I squashed the commits, I think it's ready to try it as pre-release ;)

lukkry · 2013-11-07T23:16:32Z

@osheroff should I go ahead and merge it?

osheroff · 2013-11-07T23:21:46Z

I went ahead and released 3.0.0.beta1 from this branch; try that in a real project or two before merge? I was in flight with testing it on classic when security issues got in the way, can resume next week....

lukkry · 2013-11-07T23:28:33Z

ok, cool, I'll test it against classic as well

grosser · 2014-01-20T03:06:04Z

Rakefile

@@ -2,6 +2,8 @@ require 'bundler/setup'
 require "appraisal"
 require "bump/tasks"

+Bundler::GemHelper.install_tasks


require 'bundler/gem_tasks' does the same

why is it better?

not much, but I like it better, a little shorter / just a require vs a
method call

On Mon, Jan 20, 2014 at 8:56 AM, Lukasz Krystkowiak <
notifications@github.com> wrote:

In Rakefile:

@@ -2,6 +2,8 @@ require 'bundler/setup'
require "appraisal"
require "bump/tasks"

+Bundler::GemHelper.install_tasks

why is it better?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5/files#r9010711
.

Conflicts: Gemfile active_record_shards.gemspec lib/active_record_shards/default_slave_patches.rb test/schema.rb

grosser · 2014-01-21T18:56:07Z

is this ready to go now ? I want to play with rails 4 :)

osheroff · 2014-01-21T19:00:11Z

The thing that was, and is blocking this is someone testing it with classic on acceptance.

grosser · 2014-01-21T19:07:06Z

can do!

grosser · 2014-01-21T19:09:18Z

@lukkry there should be no need to use your own fork

lukkry · 2014-01-21T20:43:39Z

@grosser I pushed it all to rails4_support branch in this repo, https://github.com/zendesk/active_record_shards/tree/rails4_support, so we can work with it, instead of the fork.

Also, it would be cool to merge #10 first, and rebase everything on top of it ;)

grosser · 2014-01-21T20:52:53Z

Tried in on tddium + acceptance and seems to work nicely.

On Tue, Jan 21, 2014 at 12:43 PM, Lukasz Krystkowiak <
notifications@github.com> wrote:

@grosser https://github.com/grosser I pushed it all to rails4_support
branch in this repo,
https://github.com/zendesk/active_record_shards/tree/rails4_support, so
we can work with it, instead of the fork.

Also, it would be cool to merge #10 https://github.com/zendesk/active_record_shards/pull/10first, and rebase everything on top of it ;)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5#issuecomment-32961132
.

grosser · 2014-01-21T21:51:01Z

done, try to rebase!

lukkry · 2014-01-21T22:25:18Z

I'm closing this in favor of #12

osheroff reviewed Aug 27, 2013
View reviewed changes

bquorning reviewed Aug 29, 2013
View reviewed changes

lukkry reviewed Sep 7, 2013
View reviewed changes

bquorning reviewed Sep 14, 2013
View reviewed changes

snarglebritchet mentioned this pull request Sep 19, 2013

ActiveRecord::Base::ConnectionSpecification doesn't exist in AR 4.0.0 OutOfOrder/multidb#4

Closed

add support for activerecord 4

0501d49

grosser reviewed Jan 20, 2014
View reviewed changes

lukkry added 2 commits January 20, 2014 16:50

don't specify a platform for 'debugger', it exists on 2.0 and 2.1

7802c85

Merge branch 'master' into rails4_support

87c5b86

Conflicts: Gemfile active_record_shards.gemspec lib/active_record_shards/default_slave_patches.rb test/schema.rb

lukkry closed this Jan 21, 2014

lukkry mentioned this pull request Jan 21, 2014

Rails 4 #12

Merged

Rails 4 #5

Rails 4 #5

Conversation

lukkry commented Aug 25, 2013

lukkry commented Aug 25, 2013

staugaard commented Aug 27, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osheroff commented Aug 27, 2013

osheroff commented Aug 27, 2013

lukkry commented Aug 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukkry commented Aug 29, 2013

lukkry commented Sep 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukkry commented Sep 14, 2013

Choose a reason for hiding this comment

osheroff commented Nov 5, 2013

lukkry commented Nov 5, 2013

lukkry commented Nov 7, 2013

osheroff commented Nov 7, 2013

lukkry commented Nov 7, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser commented Jan 21, 2014

osheroff commented Jan 21, 2014

grosser commented Jan 21, 2014

grosser commented Jan 21, 2014

lukkry commented Jan 21, 2014

grosser commented Jan 21, 2014

grosser commented Jan 21, 2014

lukkry commented Jan 21, 2014