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

Use Arc dyn BlobStore insteadl of impl #1491

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Conversation

JakeSiFive
Copy link
Contributor

This changes the BlobStore trait to be object safe while maintaining most of the generality of the previous implementation.

@JakeSiFive JakeSiFive requested a review from V-FEXrt December 14, 2023 03:54
Copy link
Contributor

@V-FEXrt V-FEXrt left a comment

Choose a reason for hiding this comment

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

Approving with the musing that this feels a lot like using Rust magic to go out of the way to avoid a copy when a copy would have been just fine

@@ -61,7 +61,9 @@ struct ServerOptions {
fn create_router(conn: Arc<DatabaseConnection>, config: Arc<config::RSCConfig>) -> Router {
// If we can't create a store, just exit. The config is wrong and must be rectified.
let root = config.local_store.clone().unwrap();
let store = blob::LocalBlobStore { root };
//let store = blob::LocalBlobStore { root };
Copy link
Contributor

Choose a reason for hiding this comment

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

commented code should be removed

@JakeSiFive
Copy link
Contributor Author

Approving with the musing that this feels a lot like using Rust magic to go out of the way to avoid a copy when a copy would have been just fine

copies don't share state and thus a) can mutate when shared across threads and b) can't update when they need to. The point is NOT to avoid the copy, this is more expensive than just doing a copy, the point is to have a single source of truth.

@JakeSiFive
Copy link
Contributor Author

Also the core core core issue that you seem to keep missing is that you CANNOT dynmaically dispatch based on the config across an impl so if you'd have to either duplicate the routing code or have other config code that uses pattern matching in order to call the correct monomorphization of the router creation code. This avoids having the issue of dispatching to different monomorphizations

@JakeSiFive JakeSiFive enabled auto-merge (squash) December 15, 2023 14:40
@JakeSiFive JakeSiFive merged commit 2db2b4b into master Dec 15, 2023
12 checks passed
@JakeSiFive JakeSiFive deleted the use_dynamic_blob_store branch December 15, 2023 14:56
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