Rails core - Nov 2005 - connection pooling patch for ActiveRecord

Hi all,

First off, thanks the invite to join this mailing list, I was  
wondering if such a thing existed while working on my patch, and I  
think its a great idea!

I recently submitted a 2nd version of my patch for connection pooling  
in active record.  This time I implemented it by redefining the  
"connect_<adapter>" methods to return a ConnectionPool::Wrapper
object, which acts like a real connection adapter but really forwards  
all requests to a real adapter that it gets from a connection pool.   
This seemed to be the least intrusive way to do it, so the rest of  
the active record code did not have to be aware of the change.  I was  
just wondering if this was a good approach or not, and also what the  
chances are of the patch being accepted.  For more details, see  
ticket #2162.

Thanks again,

Greg

> This seemed to be the least intrusive way to do it, so the rest of
> the active record code did not have to be aware of the change.  I was
> just wondering if this was a good approach or not, and also what the
> chances are of the patch being accepted.  For more details, see
> ticket #2162.
It would be great if all the changes to
activerecord/lib/active_record/base.rb happened also in the connection
pool file by reopening the base class and adding these.

Also, you can create a single patch doing "svn add file" before you do
the svn diff. That way you don''t need a separate tar.

- david

Ok, I moved the new constants from base.rb to connection_pool.rb and  
made a new diff after doing svn add for the new files.  New diff is  
attached to ticket as "active_record_connection_pooling3.diff"

Thanks!

Greg

On Nov 7, 2005, at 11:26 AM, David Heinemeier Hansson wrote:
>> This seemed to be the least intrusive way to do it, so the rest of
>> the active record code did not have to be aware of the change.  I was
>> just wondering if this was a good approach or not, and also what the
>> chances are of the patch being accepted.  For more details, see
>> ticket #2162.
>
> It would be great if all the changes to
> activerecord/lib/active_record/base.rb happened also in the connection
> pool file by reopening the base class and adding these.
>
> Also, you can create a single patch doing "svn add file" before
you do
> the svn diff. That way you don''t need a separate tar.
>
> - david
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://wrath.rubyonrails.org/pipermail/rails-core/attachments/20051107/0a319b3f/attachment.html

On 11/8/05, Greg Lappen <greg@lapcominc.com>
wrote:
> Ok, I moved the new constants from base.rb to connection_pool.rb and made a
> new diff after doing svn add for the new files.  New diff is attached to
> ticket as "active_record_connection_pooling3.diff"
This is something I''ve been meaning to raise for a while,  but when
raising exceptions,  it''d be nice if the ''cause'' was
included.

185 	+        rescue Exception => e
186 	+          log_info("error while closing connection:
#{e.message}")
187 	+          raise ActiveRecord::ActiveRecordError

While this logs the necessary info, the catching code really has no
idea what''s going on.  In StrongSpace''s case, we automatically
turn
Exceptions into bugs in FogBUGZ.   Either that, or a specific
exception say ActiveRecord::ConnectionCloseError ?

Does anyone else have any thoughts on the matter?

My only other comments on the patch would be that the tests are a
little hard to follow, perhaps some comments explaining what''s going
on?

--
Cheers

Koz

Ok, made these changes and attached new code to ticket as  
"active_record_connection_pooling4.diff"

Thanks for the feedback!

Greg
On Nov 7, 2005, at 3:00 PM, Michael Koziarski wrote:
> On 11/8/05, Greg Lappen <greg@lapcominc.com> wrote:
>> Ok, I moved the new constants from base.rb to connection_pool.rb  
>> and made a
>> new diff after doing svn add for the new files.  New diff is  
>> attached to
>> ticket as "active_record_connection_pooling3.diff"
>
> This is something I''ve been meaning to raise for a while,  but
when
> raising exceptions,  it''d be nice if the ''cause''
was included.
>
> 185 	+        rescue Exception => e
> 186 	+          log_info("error while closing connection: # 
> {e.message}")
> 187 	+          raise ActiveRecord::ActiveRecordError
>
> While this logs the necessary info, the catching code really has no
> idea what''s going on.  In StrongSpace''s case, we
automatically turn
> Exceptions into bugs in FogBUGZ.   Either that, or a specific
> exception say ActiveRecord::ConnectionCloseError ?
>
> Does anyone else have any thoughts on the matter?
>
> My only other comments on the patch would be that the tests are a
> little hard to follow, perhaps some comments explaining what''s
going
> on?
>
> --
> Cheers
>
> Koz
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://wrath.rubyonrails.org/pipermail/rails-core/attachments/20051107/51927ac2/attachment.html