indexer: refactor migration logic to use an async connection #19259
Conversation
bmwill
requested review from
amnn,
wlmyng,
stefan-mysten,
emmazzz and
suiwombat
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
crates/sui-indexer/src/db.rs
Outdated
| info!("Dropped all functions."); | ||
|
|
||
| conn.setup()?; | ||
| info!("Created __diesel_schema_migrations table."); | ||
| // conn.setup()?; |
There was a problem hiding this comment.
Is this intentional? the setup call creates the diesel record table. I guess you could revert it back to explicit sql if it's hard to turn it into async
There was a problem hiding this comment.
I don't believe this explicit call to setup is necessary, without it all the tests still pass properly. Its likely handled as a part of the run_pending migrations flow. I'll remove the commented out line though
| reset_database(pool.dedicated_connection().await.unwrap()) | ||
| .await | ||
| .unwrap(); | ||
| check_db_migration_consistency(&mut pool.get().await.unwrap()) |
There was a problem hiding this comment.
Is there a reason one uses dedicated while the other use non-dedicated?
There was a problem hiding this comment.
yes, reset_database requires an owned connection that is passed by value while the consistency check just needs one by &mut. This is due to needing to send the owned connection to a blocking thread to run the migrations.
bmwill
force-pushed
the
indexer-async-migrations
branch
from
c34339b
to
13f20b8
Compare
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.