kenn / standby

Read from standby databases for ActiveRecord
MIT License
87 stars 28 forks source link

Make the slave_connection also thread local #5

Closed jayuen closed 7 years ago

jayuen commented 9 years ago

Hi there:

I wanted to get your thoughts on this PR. Basically, we made the slave_connection variable thread local for a couple reasons:

1) Within a thread, we are making multiple calls to the slave via different ActiveRecord classes (e.g. through find_by_sql). Therefore, we didn't want to create a new slave connection each time.

2) Since the Slavery#on_slave method uses a thread variable, maintaining a single slave connection for the thread seemed to be consistent.

Appreciate your comments, Jason

kenn commented 9 years ago

If my understanding is (still) correct, establish_connection does not actually establish a connection, but just assigns a connection pool.

And a connection is actually checked out from the pool when ActiveRecord::Base.connection is called (e.g. find_by_sql), which creates a new connection in the pool if it isn't initialized already.

In other words, it respects your "pool: 5" config in database.yml, and it shouldn't create a new connection every time.

Do you have an evidence that proves my theory wrong? In any case, making connection(s) thread-local doesn't make sense to me, as it conflicts with the way it is implemented by the connection pool.

kenn commented 7 years ago

Completely reworked with v2, so please have a look. I believe this has never been an issue though.