Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

Use Key-value DB for DataStore and PinStore #439

Merged
merged 2 commits into from
Jan 5, 2021
Merged

Use Key-value DB for DataStore and PinStore #439

merged 2 commits into from
Jan 5, 2021

Conversation

fetchadd
Copy link
Contributor

@fetchadd fetchadd commented Jan 5, 2021

This PR is for using key-value db like sled as DataStore and PinSotre, so that it is more efficient for pinning and easy to realize MFS.

The FsDatastore is inefficiency now, when checking a indirect pinned cid, all file in the FsDatastore will be visited and read, what more important is that it is hard to distinguish the data for pinning and the data for DataStore. The key-value db like sled will bring convenience and efficiency in the next work with its high efficiency and easy key/value operation. Leveldb may be the first choice of key-value dbs, but now there are no suitable rust leveldb crates; Sled is fast and simple, and it is usable on servers and phones from any C-compatible language.

This PR is the reopen of PR434. For there is some verbose commits like "replaceing leveldb with sled", the master branch of fetchadd/rust-ipfs in the PR434 is deprecated. Now the same changes are applied to the staging branch which is synced with the master of rust-ipfs/rust-ipfs. So Thanks for the PR of fetchadd/rust-ipfs from @koivunej which uses mutex to work around init(&self) and unsafe, but as the master is deprecated, I can only copy the code to staging branch.

Accorrding to the review from @ koivunej in PR434, the pinstore_interface_tests is added to KvDataStore, and now all tests are passed.

Closes #434.

Copy link
Collaborator

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

This is looking great, thank you. I might be adding a few stylistic tweaks, changelog and comments and will be cc'ing you in a follow-up PR.

Closes #434. Nope this didn't work, adding it to the description.

Awaiting a sec @ljedrz wants to review as well..

src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Show resolved Hide resolved
src/repo/kv.rs Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
src/repo/kv.rs Outdated Show resolved Hide resolved
let db = kv_db.get_db();

match db.get(key.as_str()) {
Ok(Some(_)) => return Ok(Some(mode.clone())),
Copy link
Member

@ljedrz ljedrz Jan 5, 2021

Choose a reason for hiding this comment

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

note: PinMode isn't Copy, but it should be

Copy link
Collaborator

Choose a reason for hiding this comment

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

very much agreed.

src/repo/kv.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link
Member

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@koivunej
Copy link
Collaborator

koivunej commented Jan 5, 2021

Thank you very much @fetchadd!

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 5, 2021

Build succeeded:

@bors bors bot merged commit fdaf3a5 into rs-ipfs:master Jan 5, 2021
@koivunej
Copy link
Collaborator

koivunej commented Jan 5, 2021

@fetchadd re: my review comments; I found a few things to fix, missing tests but I'll post a draft on thursday on my progress.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants