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

Remove execution for connection manager #52

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Remove execution for connection manager #52

merged 3 commits into from
Sep 29, 2023

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Sep 25, 2023

  • In an attempt to simplify errors, reduce polymorphism, and reduce build times, this PR removes support for "pool-based queries", instead preferring that clients check out connections before sending queries to the underlying pool.

Previously

  • AsyncConnection was implemented for both connections and for the pool itself. This meant the query execution functions (i.e., execute_async(...)) could act on either a pool or a connection.
  • Although this was flexible, it was arguably too flexible -- clients would need to deal with ConnectionError errors from queries to connections, or PoolError errors from queries to pools (which is basically ConnectionError + Any error from checking out the connection).

Now

  • AsyncConnection is implemented for connections, not pools. This should unify error handling for callers, and hopefully make the access to individual DB connections a little more obvious.

Part of oxidecomputer/omicron#4132

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

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

Looks good by itself, though I'd wait for the corresponding Omicron PR to be ready to land before landing this one (in case we find problems there).

Out of curiosity, do other crates impl Connection for the bb8 pool or was that just something we did?

@smklein
Copy link
Collaborator Author

smklein commented Sep 25, 2023

Looks good by itself, though I'd wait for the corresponding Omicron PR to be ready to land before landing this one (in case we find problems there).

Sounds good, I'll wait until presubmit CI goes green.

Out of curiosity, do other crates impl Connection for the bb8 pool or was that just something we did?

I really thought I remembered other crates doing this, but I can't find other good examples where "the pool also implements the connection type".

https://docs.rs/diesel/latest/diesel/prelude/trait.Connection.html is implemented on "Connection" types, that aren't pools.

I took some inspiration from https://docs.rs/bb8-diesel/ when building this crate, and there, https://docs.rs/bb8-diesel/0.2.1/bb8_diesel/struct.DieselConnectionManager.html does not implement the Connection trait.

@smklein smklein merged commit da04c08 into master Sep 29, 2023
@smklein smklein deleted the less-pool branch September 29, 2023 16:50
smklein added a commit to oxidecomputer/omicron that referenced this pull request Sep 29, 2023
- Accesses the DB via connections explicitly, rather than using a pool
- This change reduces generics slightly, and simplifies error handling.
Callers can deal with "pool errors" when checking out the connection,
and can deal with "query errors" separately when issuing the queries
themselves.

Depends on oxidecomputer/async-bb8-diesel#52
Fixes #4132
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.

2 participants