Commits on Dec 4, 2019
Commits on Dec 3, 2019
The `database` kwarg in `connected_to` has resulted in a lot of bug reports that are trying to use it for sharding when that's not the intent of the key. After considering where the database kwarg is used in tests and thinking about usecases for it, we've determined it should be removed. There are plans to add sharding support and in the mean time the database kwarg isn't the right solution for that. Applications that need to create new connections can use establish_connection or connects_to. Since the database key causes new connections to be established on every call, that causes bugs if connected_to with a database kwarg is used during a request or for any connection that's not a one-off. Co-authored-by: John Crepezzi <firstname.lastname@example.org>
If a spec name was provided without an env name, config_for would return the first config that matched the spec, regardless of environment name. Now configs_for will return the database config that matches the default env and requested spec name. Additionally this commit has moved the default env call into a method because I'm tired of typing so many lines every single time. We considered either returning all configs that match that spec name or raising an error if only spec was passed, but this change has the least impact on current behavior and matches Active Record's assumptions: that if you ask for configs it will always consider the current environment. Co-authored-by: John Crepezzi <email@example.com>
Commits on Nov 26, 2019
Commits on Nov 25, 2019
Commits on Nov 22, 2019
Commits on Nov 21, 2019
Commits on Nov 16, 2019
Nov 16, 2019
JRuby can't support processes, and in memory db can't support changing the handlers/connections during the test.
Commits on Nov 15, 2019
Nov 15, 2019
…ng processes In a forking process web server like Unicorn, connections are reconnected after fork. This test ensures that when connects are reconnected after fork when using multiple databases, the query cache will be on and work correctly. This test fails on Rails 6.0 for now.
Commits on Nov 13, 2019
Commits on Nov 7, 2019
This commit renames `RoleManager` -> `PoolManager` and `Role` -> `PoolConfig`. Once we introduced the previous commit, and looking at the existing code, it's clearer that `Role` and `RoleManager` are not the right names for these. Since this PR moves away from swapping the connection handler concepts around and the role concept will continue existing on the handler level, we need to rename this. A `PoolConfig` holds a `connection_specification_name` (we may rename this down the road), a `db_config`, a `schema_cache`, and a `pool`. It does feel like `pool` could eventually hold all of these things instead of having a `PoolConfig` object. This would remove one level of the object graph and reduce complexity. For now I'm leaving this object to keep the change churn low and will revisit later. Co-authored-by: John Crepezzi <firstname.lastname@example.org>
Commits on Nov 5, 2019
This PR is an alternate solution to #37388. While there are benefits to merging #37388 it changes the public API and swaps around existing concepts for how connection management works. The changes are backwards-incompatible and pretty major. This will have a negative impact on gems and applications relying on how conn management currently works. **Background:** Shopify and other applications need sharding but Rails has made it impossible to do this because a handler can only hold one connection pool per class. Sharded apps need to hold multiple connections per handler per class. This PR aims to solve only that problem. **What this PR does:** In this PR we've added a `RoleManager` class that can hold multiple `Roles`. Each `Role` holds the `db_config`, `connection_specification_name`, `schema_cache` and `pool`. By default the `RoleManager` holds a single reference from a `default` key to the `Role` instance. A sharded/multi-tenant app can pass an optional second argument to `remove_connection`, `retrieve_connection_pool`, `establish_connection` and `connected?` on the handler, thus allowing for multiple connections belonging to the same class/handler without breaking backwards compatibility. By using the `RoleManager` we can avoid altering the public API, moving around handler/role concepts, and achieve the internal needs for establishing multiple connections per handler per class. **A note about why we opened this PR:** We very much appreciate the work that went into #37388 and in no way mean to diminish that work. However, it breaks the following public APIs: * `#retrieve_connection`, `#connected?`, and `#remove_connection` are public methods on handler and can't be changed from taking a spec to a role. * The knowledge that the handler keys are symbols relating to a role (`:writing`/`:reading`) is public - changing how handlers are accessed will break apps/libraries. In addition it doesn't solve the problem of mapping a single connection to a single class since it has a 1:1 mapping of `class (handler) -> role (writing) -> db_config`. Multiple pools in a writing role can't exist in that implementation. The new PR solves this by using the `RoleManager` to hold multiple connection objects for the same class. This lets a handler hold a role manager which can hold as many roles for that writer as the app needs. **Regarding the `Role` name:** When I originally designed the API for multiple databases, it wasn't accidental that handler and role are the same concept. Handler is the internal concept (since that's what was there already) and Role was the public external concept. Meaning, role and handler were meant to be the same thing. The concept here means that when you switch a handler/role, Rails automatically can pick up the connection on the other role by knowing the specification name. Changing this would mean not just that we need to rework how GitHub and many many gems work, but also means retraining users of Rails 6.0 that all these concepts changed. Since this PR doesn't move around the concepts in connection management and instead creates an intermediary between `handler` and `role` to manage the connection data (`db_config`, `schema_cache`, `pool`, and `connection_specification`) we think that `Role` and `RoleManager` are the wrong name. We didn't change it yet in this PR because we wanted to keep change churn low for initial review. We also haven't come up with a better name yet.
😄**What this PR does not solve:** Our PR here solves a small portion of the problem - it allows models to have multiple connections on a class. It doesn't aim to solve any other problems than that. Going forward we'll need to still solve the following problems: * `DatabaseConfig` doesn't support a sharding configuration * `connects_to`/`connected_to` still needs a way to switch connections for shards * Automatic switching of shards * `connection_specification_name` still exists **The End** Thanks for reading this far. These problems aren't easy to solve. John and I spent a lot of time trying different things and so I hope that this doesn't come across as if we think we know better. I would have commented on the other PR what changes to make but we needed to try out different solutions in order to get here. Ultimately we're aiming to change as little as the API as possible. Even if the handler/role -> manager -> db_config/pool/etc isn't how we'd design connection management if we could start over, we also don't want to break public APIs. It's important that we make things better while maintaining compatibility. The `RoleManager` class makes it possible for us to fix the underlying problem while maintaining all the backwards compatibility in the public API. We all have the same goal; to add sharding support to Rails. Let me know your thoughts on this change in lieu of #37388 and if you have questions. Co-authored-by: John Crepezzi <email@example.com>
Commits on Nov 1, 2019
Commits on Oct 22, 2019
Commits on Oct 21, 2019
Commits on Oct 18, 2019
In order to move schema_cache off of DatabaseConfiguration we needed to make role accessible on pool. While looking at `ConnectionSpecification`, `Role`, and `DatabaseConfig` John and I noticed that this could be achieved by merging `ConnectionSpecification` and `Role` into one `Role` class. This allows us to eliminate the `spec` concept which is confusing. `spec` is a private method so renaming to `resolve_role` is ok. In the `Role` class we took `name` (renamed to `connection_specification_name` for clarity since it's not a `role` name) and `db_config` from `ConnectionSpecification` and the `pool` methods from `Role` and combined them into one `Role` class. This feels a lot cleaner to us because it clarifies the purposes of the classes/methods/variables, and makes it easier to drop `connection_specification_name` keyed on the parent class in the future. There are a lot of changes in here but the majority of them are find and replace `spec` -> `role`, `spec.name` -> `role.connection_specification_name`, `Resolver#spec` -> `Resolver#resolve_role`. This PR also moves the `schema_cache` from `DatabaseConfig` to the new combined `Role` class. Co-authored-by: John Crepezzi <firstname.lastname@example.org>
Commits on Oct 16, 2019
Commits on Oct 14, 2019
This reverts commit f54a8d0. I am so sorry. I accidentally applied this on top of another PR to do some testing and then when I merged that one this got merged. I'm reverting this because we don't need to revert this now.
Commits on Oct 11, 2019
We have these nice objects for collecting database configurations, so we should use them everywhere instead of the hashes. Also call `db_config.database` and `db_config.adapter` where necessary. John Crepezzi <email@example.com>
This reverts commit 4ea7769, reversing changes made to 2ef9eca. I'm reverting this PR for now at Rafael's recommendation because some of the changes have caused our Rails upgrade at GitHub to be blocked (we use some private API's) as well as other work to use the db_config to establish a connection. See PR and comments here #37408 for more detail.
Commits on Oct 7, 2019