Update diesel, diesel_migrations, uuid, and rand #132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jan 10, 2023
primeos-work
force-pushed
the
diesel-update
branch
2 times, most recently
from
08771a2 to
619021e
Compare
This implements the major upgrade of the Diesel crate from version 1 to
version 2. The relevant changes are the following:
- Diesel now requires mutable access to the `Connection` to perform any
database interaction [0].
- The Diesel derive attributes need to be wrapped by `diesel()` now [1].
- The diesel_migration crate has been completely rewritten and the
`embed_migrations!()` macro had to be replaced [2].
- The `connection::Connection::transaction` closure now takes one
argument (the database connection). See [3] vs. [4].
- I also decided to rename `database_connection` to `conn` in these
cases to better differentiate the two (we could keep/shadow the
outer variable for a smaller diff but it seems cleaner that way).
Note: The `Arc<PgConnection> -> Arc<Mutex<PgConnection>>` change is only
intended as a PoC. It is not a good idea as the mutex locking makes some
of the async blocks/code completely pointless. This problem already
existed before though. The documentation sates that the requirement of
mutable connections is expected to be a "straightforward change as the
connection already can execute only one query at a time". So this change
should only make matters worse by moving the locking up and therefore
increasing the time for which locks are held. This should only make the
execution a bit slower (not sure if it would even be noticeable) but it
increases the risk of deadlocks which is the real danger.
To avoid this issue we should use a connection pool instead (e.g., via
r2d2) but there are alternatives like trying to move the locking further
down in the code again or rewriting the database logic.
[0]: https://diesel.rs/guides/migration_guide.html#2-0-0-mutable-connection
[1]: https://diesel.rs/guides/migration_guide.html#2-0-0-derive-attributes
[2]: https://diesel.rs/guides/migration_guide.html#2-0-0-upgrade-migrations
[3]: https://docs.diesel.rs/2.0.x/diesel/connection/trait.Connection.html#method.transaction
[4]: https://docs.diesel.rs/1.4.x/diesel/connection/trait.Connection.html#method.transaction
Signed-off-by: Michael Weiss <[email protected]>
The diesel+uuid part is resolved now (13bcbde) but the crate is also a direct dependency since 8706494 as src/commands/build.rs uses it for shuffling the endpoint configurations (it should be trivial to change that code though). Signed-off-by: Michael Weiss <[email protected]>
This replaces the mutex-based PoC of 13bcbde via the `r2d2` feature of the `diesel` crate. Using `unwrap()` should be fine as `Pool::get()` only errors if the code runs into a timeout (in which case there's probably a network issue or the database is down so we could only return a proper error message instead of panicking). The `min_idle` setting defaults to `None` which causes the `max_idle` value to be used (defaults to 10). The `build()` function "will block until the pool has established its configured minimum number of connections". Therefore, it seems best to lower the value to 1 since butido is a CLI and not a service/daemon (so the pool will open additional connections on demand for the async code instead of establishing all connections at the beginning - we probably only need few database connections in most cases). Signed-off-by: Michael Weiss <[email protected]>
primeos-work
force-pushed
the
diesel-update
branch
from
619021e to
1f63fc9
Compare
Now, we finally don't have any completely frozen dependencies anymore and are only holding the `config` crate back (we could continue waiting for the major rewrite of the crate that might happen or just update the code now and then again after the "rewrite" is complete). Signed-off-by: Michael Weiss <[email protected]>
primeos-work
changed the title
This is just the result of running `cargo update` without touching Cargo.toml (since we currently only use dependabot to update direct dependencies and not indirect/transitive dependencies). Version 0.1.1 of the time-core crate requires rustc 1.65.0 or newer so let's also bump the MSRV from 1.64.0 to 1.65.0. Signed-off-by: Michael Weiss <[email protected]>
primeos-work
force-pushed
the
diesel-update
branch
from
db38ae1 to
ae9d1a0
Compare
|
We did just test this PR together (against the test DB, an empty DB, mixed with an old butido client, and finally against the production DB). We didn't notice any issues/regressions so we'll merge this now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the last four crates that were restricted to outdated versions (apart from the
configcreate which we decided not to update for now). The details are described in the individual commit messages. We have to bump the MSRV as well for the last commit (updating all crates in Cargo.lock).Not much to see yet - still very incomplete.PoC (untested) is ready
but we should certainly avoid the mutexes(replaced via r2d2 connection pools).Will supersede #81, #66, and #62.
TODO: Probably update-> donerandas well (s. comment in Cargo.toml).Reason for the MSRV bump: