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

[nexus] Access DB via "connection", not "pool" #4132

Closed
smklein opened this issue Sep 21, 2023 · 0 comments · Fixed by #4140
Closed

[nexus] Access DB via "connection", not "pool" #4132

smklein opened this issue Sep 21, 2023 · 0 comments · Fixed by #4140
Assignees
Labels
database Related to database access development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better

Comments

@smklein
Copy link
Collaborator

smklein commented Sep 21, 2023

This is a gnarly implementation detail that arose out of a poor decision I made back in async-bb8-diesel.

In async-bb8-diesel, we define a trait named AsyncConnection, which allows callers to execute a variety of asynchronous Diesel operations. This trait is implemented for both the connection pool itself, as well as individual connections.

This was intended for developer convenience -- if you wanted to use a "pool" object directly to issue operations, you could. If you wanted to use a "connection" object to issue operations, you could check it out of the pool and use that too.

This approach has a couple of downsides:

  • Generics. This hits build times, causes more codegen, etc. We should use only one implementation.
  • Worse errors. Since acting on a "pool" can return a PoolError (e.g., did you fail to check out a connection?) in addition to the diesel errors (e.g. did your query fail?) it forces callers to also be generic over the type of error being used.

This can culminate in code like the following:

pub(crate) async fn service_upsert_conn<ConnError>(
&self,
conn: &(impl AsyncConnection<DbConnection, ConnError> + Sync),
service: Service,
) -> CreateResult<Service>
where
ConnError: From<diesel::result::Error> + Send + 'static,
PoolError: From<ConnError>,
{

If we universally just "checked out connections before using them", that code could be written as:

    pub(crate) async fn service_upsert_conn(
        &self,
        conn: ConcreteConnectionType,
        service: Service,
    ) -> CreateResult<Service> {

(and hey, look, that has no generics, will be faster to compile, and will be easier to reason about!)

@smklein smklein added database Related to database access development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better labels Sep 21, 2023
@smklein smklein self-assigned this Sep 21, 2023
smklein added a commit that referenced this issue 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
database Related to database access development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant