Skip to content

Convert all usage of 'transaction' to 'transaction_async'#1621

Merged
smklein merged 5 commits into
mainfrom
try-async-txn
Sep 12, 2022
Merged

Convert all usage of 'transaction' to 'transaction_async'#1621
smklein merged 5 commits into
mainfrom
try-async-txn

Conversation

@smklein

@smklein smklein commented Aug 19, 2022

Copy link
Copy Markdown
Collaborator

This PR is dependent on oxidecomputer/async-bb8-diesel#33 , and acts as a demonstration of asynchronous transaction blocks.

This PR should be a big step forward for eliminating "special-case synchronous code" to access the DB within transactions.

Whenever someone reaches for an interactive transaction, we'd prefer:

  • Pipelined transactions (not fully implemented, does not cover cases where we read-then-write)
  • Sagas (for DB-only operations, this is technically wasteful if we could do the same calculations in SQL)
  • CTEs (this is generally the "ideal" solution, but bears the highest complexity)

however, given that we are trying to strike a balance between correctness, speed, safety, and developer productivity, this PR provides a cheap way for the "fastest option" - just writing normal async code in rust - to remain async, and interoperate with the rest of the codebase easily.

Comment on lines +337 to +338
conn: &(impl async_bb8_diesel::AsyncConnection<DbConnection, ConnErr>
+ Sync),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Basically all of our non-transaction code acts on a "bb8::Pool". However, the transaction_async method expects to act on an async_bb8_diesel::Connection.

Fortunately, async_bb8_diesel::AsyncConnection is implemented on both objects, so if we operate on that interface, we can interop with either the pool or a connection object in a transaction context. This pattern of impl AsyncConnection is used in a few spots in this patch, and is how I expect we could make code "generic to being run in a txn" in the future.

Unfortunately, the errors are different - we only propagate "pool-related errors" if you're actually acting on a pool - so that is made generic here too (see: ConnErr). If being generic over errors is only a pain, and not particularly useful, we could consider just propagating a PoolError everywhere (it's a superset of the other errors that could be returned from async_bb8_diesel).

@jmpesp jmpesp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@smklein smklein merged commit c05f3b5 into main Sep 12, 2022
@smklein smklein deleted the try-async-txn branch September 12, 2022 03:36
leftwo pushed a commit that referenced this pull request Feb 10, 2025
Crucible changes are:
Nexus notifications have different importance (#1621)

Propolis changes are:
lib: don't send CPUID settings in vCPU device state payload (#848)
server/lib: add generic hypervisor enlightenment interface (#846)
server: read host CPUID values if the spec has none (#844)

Added a new field to the Board struct that propolis requires.
leftwo added a commit that referenced this pull request Feb 10, 2025
Crucible changes are:
Nexus notifications have different importance (#1621)

Propolis changes are:
lib: don't send CPUID settings in vCPU device state payload (#848)
server/lib: add generic hypervisor enlightenment interface (#846)
server: read host CPUID values if the spec has none (#844)

Added a new field to the Board struct that propolis requires.

Co-authored-by: Alan Hanson <alan@oxide.computer>
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