-
Notifications
You must be signed in to change notification settings - Fork 29
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
rsc: Update the tool to fully bootstrap a fresh db from one command #1546
Conversation
75d7127
to
51fc46b
Compare
@@ -407,6 +417,8 @@ mod tests { | |||
async fn create_test_store( | |||
db: &DatabaseConnection, | |||
) -> Result<Uuid, Box<dyn std::error::Error>> { | |||
database::create_dbonly_blob_store(db).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised we don't have to update some check now that the database has two active stores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the dbonly store does muddy the terminology a bit, but there is still only one "active" store.
The active store is just the store that, by default, a blob is pushed to. The dbonly store is always enabled but not the default.
Happy to use a different term if you have any suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test that this default dbonly blob store is working properly?
Regarding naming it seems like active
store makes sense from the perspective of someone pushing blobs, but from someone pulling blobs they may get results from "inactive" stores. default
also doesn't solve this confusion. It seems like "push" or "write" or some verb like that would need to be joined with the other adjective, so "default_push_store" or "active_write_store" or things like this but I'm not sure any of those combinations actually provide clarity, and may impair understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test that this default dbonly blob store is working properly?
No not yet, I'll get one added in a followup. Testing the blob uploads is a bit more tricky since I need to stream files but it should be done
database::create_dbonly_blob_store(db).await?; | ||
|
||
println!(""); | ||
println!("Done! Use the store id to set the active store and share the key as appropiate."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the active store is something that user does via another rsc_tool invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a part of the launch parameters into the RSC. You aren't allowed to change the active store while the RSC is running since the "activation" of a store is a non trivial action on a nearly global hashmap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably add a README to rsc_tool
to cover common use patterns that aren't covered by --help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A couple of comments for things that can be addressed in a follow up PR.
rsc_tool --bootstrap
now completes all tasks needed to bootstrap an empty but migrated postgres database. This includes supporting DbOnly Blob Store which was previously under supported. There is now only one place where the DbOnly Blob Store id is hard coded.