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

Delay DB connecting until it becomes necessary #32

Merged
merged 2 commits into from
May 21, 2018

Conversation

a2ikm
Copy link
Contributor

@a2ikm a2ikm commented May 16, 2018

Currently Tako establishes connections with shards at booting up.
This PR changes it to at executing first query.

Furthermore, this enables refreshing connections by each request with activerecord-refresh_connection.

Note that these changes are made by @hoco, who is my co-worker.
Thanks @hoco!

@the40san
Copy link
Owner

🤔
Should we really need refresh connection by each request?
Fmm? Perhaps it is Architecture or resource-dependent issue.

@the40san
Copy link
Owner

btw connection timing change is good idea.

proxy_configs[shard_name] = conf
end

def create_proxy(shard_name)
Proxy.new(shard_name, proxy_connections[shard_name.to_sym])
Proxy.new(shard_name, proxy_classes[shard_name.to_sym].connection_without_tako)
Copy link
Owner

Choose a reason for hiding this comment

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

Does it makes any change? :)

Copy link
Contributor Author

@a2ikm a2ikm May 21, 2018

Choose a reason for hiding this comment

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

Please note that calling ActiveRecord::ConnectionHandling#connection connects with the database.

Currently Tako::Repository#add calls it.
With these changes, Tako::Repository#add doesn't, but Tako::Repository#create_proxy does.
So a connection is created at querying with a new proxy.

Copy link
Owner

Choose a reason for hiding this comment

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

Do we need alias_method for that?

Copy link
Contributor Author

@a2ikm a2ikm May 21, 2018

Choose a reason for hiding this comment

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

Ah, I see what you mean.
Yes, we need it because the connection argument provided to Tako::Proxy#initialize can be affected by Tako::ProxyStack.top.

With the original code, temporary_class.connection in add method always returns the result of super in Tako::ActiveRecordExt::ConnectionHandling#connection because Tako::ProxyStack.top is nil when loading shards.yml.

module Tako
  module ActiveRecordExt
    module ConnectionHandling
       def connection
         return Tako::Repository.create_proxy(@force_shard).connection if force_shard?
         if Tako::ProxyStack.top
           Tako::ProxyStack.top.connection
         else
           super # Always comes here when called within `add` method
         end
       end

With the new code, proxy_classes[shard_name.to_sym].connection in create_proxy method can return a result of both super or Tako::ProxyStack.top.connection because Tako::ProxyStack.top can be non-nil. If it returns latter, shard's name and real connection conflict in a proxy object. This is same if force_shard? is true .

For example, without the alias, this code creates a proxy object where shard_name is :shard02 but connection is to shard01.

Tako.shard(:shard01) { ModelA.shard(:shard02).where(value1: 1).limit(5).count }

So we need to call the original connection method to reduce effects of Tako::ProxyStack.top.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahhhh!!! I got understand. In the sample case.

Tako.shard(:shard01) { ModelA.shard(:shard02).where(value1: 1).limit(5).count }

if ModelA.shard(:shard02) is first query, it might be go to :shard01 because of nested w/o alias...
Must be fixed.

and there are some refactor-able magic code...!

@the40san
Copy link
Owner

Thanks I understood whole things

@the40san
Copy link
Owner

👍

@the40san the40san merged commit c22610f into the40san:master May 21, 2018
@a2ikm
Copy link
Contributor Author

a2ikm commented May 21, 2018

Thank you!

there are some refactor-able magic code...!

Yeah, I think so too :)

@a2ikm a2ikm deleted the lazy_connection branch May 21, 2018 12:55
@the40san the40san mentioned this pull request May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants