-
Notifications
You must be signed in to change notification settings - Fork 24
feat(firewood/ffi): add FjallStore
#1395
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
Conversation
|
Note to reviewers: I've marked the PR as |
rkuris
left a comment
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 not sure I love changing the reaping logic to fix this issue. The commit should have the ability to reap any revision at any time.
I think there are a few possible approaches here:
1 - Part of reaping is adding it to the RootStore
2 - Every commit adds to RootStore so entries are both in the RootStore and in the revision manager (preferred).
I think we determined that this PR does not affect performance, so I think this is mergeable at this point. |
rkuris
left a comment
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 not sure I love the separation of concerns here, but I could be convinced it's okay.
Perhaps the Rootstore interface should be that it returns nodestores themselves, and then the RootStore can deal with its own caching (only in the real implementation).
I think that cleans up the manager itself to focus on doing what it's supposed to be doing (tracking nodestores) instead of constructing them.
|
@rkuis I'm definitely on-board with having stateDiagram-v2
s1: Revision Manager
s2: RootStore
s1 --> s2: Wants historical revision X
s2 --> s1: Wants latest revision
One way to fix this would be to pass in the latest revision to fn get(
&self,
hash: &TrieHash,
latest_revision: CommittedRevision,
) -> Result<Option<LinearAddress>, Box<dyn std::error::Error + Send + Sync>>;However, passing in the latest revision seems odd (we expect pub struct InMemoryRevisions {
historical: VecDeque<CommittedRevision>,
by_hash: HashMap<TrieHash, CommittedRevision>
}Refactoring our code to use this DS would lead to the following dependency graph: stateDiagram-v2
s1: Revision Manager
s2: RootStore
s3: InMemoryRevisions
s1 --> s3: Maintains latest revisions
s1 --> s2: Wants historical revision
s2 --> s3: Wants latest revision
|
Let's enter a new issue and do this. We should also have methods on this struct to manage and fetch from the list. |
rkuris
left a comment
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.
Approved on creation of a followup for simplifying management of in memory revisions.
|
Created #1470 which will track this. |
This PR adds
FjallStore, an implementation ofRootStorethat allows for persistent storage of revisions (in contrast toMockStore, which keeps everything in-memory). The main changes of this PR are as follows:FjallStoreimplementationDbConfigto take in a path to whereFjallStorewill be storedFjallStoretests in both Rust and Golang