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

Jump to bottom

Server: Allow choice of storage backend with feature #60

Merged
merged 1 commit into from Sep 19, 2020
Merged

Server: Allow choice of storage backend with feature #60

merged 1 commit into from Sep 19, 2020

Conversation

theduke
Copy link
Contributor

@theduke theduke commented Sep 18, 2020

No description provided.

@theduke theduke force-pushed the server-storage-feature branch 2 times, most recently from 5d46554 to 60b7049 Compare September 18, 2020 22:34
Tpt
Tpt requested changes Sep 19, 2020
View reviewed changes
Copy link
Collaborator

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

Thank you so much for this pull request!

There is just a problem with a use directive in a test.

server/src/main.rs Outdated
@@ -441,7 +442,7 @@ mod tests {
use crate::handle_request;
use async_std::task::block_on;
use http_types::{Method, Request, StatusCode, Url};
use oxigraph::RocksDbStore;
use oxigraph::Store;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sadly, it does not work because Rust is looking for Store in the oxigraph crate directly.

I believe this works:

Suggested change
use oxigraph::Store;
use super::Store;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: always run the tests yourself. ;)

@theduke
server: allow storage backend choice with feature
7e09b10 
Implement rocksdb and sled features for the oxigraph_server crate.
@Tpt Tpt merged commit 61a2735 into oxigraph:master Sep 19, 2020
@Tpt
Copy link
Collaborator

Tpt commented Sep 19, 2020

Thank you!

@theduke
Copy link
Contributor Author

theduke commented Sep 19, 2020

As a side note: it would have been cleaner to make the code generic over MutableDatastore instead, but the sophia type signatures are pretty complex so I took the fast path.

@Tpt
Copy link
Collaborator

Tpt commented Sep 19, 2020

I should maybe introduce a MutableDatastore like trait in Oxigraph that would allow to do that easily. I have not done it yet because I did not not figured out a way to do it in a nice way without GAT. But I have simplified quite a lot the Oxigraph APIs in the past few month so it's maybe doable now. Having this trait might also help remove the code duplication between the different stores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tpt Tpt Tpt requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@theduke @Tpt