openraft
openraft copied to clipboard
datafuselabs
Feature: Added sled store example based on rocks example
Hey, this adds a sled db example based on rocks db example as mentioned in #418. New example are at-par functionality wise with current but achieve it in different ways showcasing a different set of libraries to achieve comparable results.
Checklist
- [ ] Updated guide with pertinent info (may not always apply).
- [x] Squash down commits to one or two logical commits which clearly describe the work you've done.
- [x] Unittest is a friend:)
This change is 
Thanks, man!
AFAIK, there is still a concern with multiple sled instances:
From the sled README: https://github.com/spacejam/sled
sled does not support multiple open instances for the time being. Please keep sled open for the duration of your process's lifespan. It's totally safe and often quite convenient to use a global lazy_static sled instance, modulo the normal global variable trade-offs. Every operation is threadsafe, and most are implemented under the hood with lock-free algorithms that avoid blocking in hot paths.
We had encountered problems with multiple sled instances in our project and have switched to a single-instance manner.
To use sled, there has to be a singleton sled instance, as it does: https://github.com/datafuselabs/databend/blob/ee1d5c023ddb526744650786dfc5f45668f78d22/common/meta/sled-store/src/db.rs#L74-L83
Since the example framework can be reused with different store engines, this example can be just a Raftstorage implementation, to reduce code duplication.
The rocksdb store is going to be a standalone crate:
- #440
With standalone crate i think its good to wait for https://github.com/datafuselabs/openraft/pull/440 to be merged.
As for the singleton from what i understand only place where db instance is created is in new function that should be called once per raft instance. Created db like that is owned by storage and it lives as long as storage lives which practically will be for the duration of program. It was done to reduce exposed types from sled crate.
Would you prefer that new function accepts sled::Db as parameter instead of directory path?
This is cool :D, we used sled in the past it's a super interesting project!
With standalone crate i think its good to wait for #440 to be merged.
Right:D
As for the singleton from what i understand only place where db instance is created is in new function that should be called once per raft instance. Created db like that is owned by storage and it lives as long as storage lives which practically will be for the duration of program. It was done to reduce exposed types from sled crate.
I mean, sled does not allow to create multi instances in a process. The concurrency control in it is process-wise. In the example, it creates multiple instances in threads, which may lead to some concurrency issues, according to the doc.
Would you prefer that new function accepts sled::Db as parameter instead of directory path?
Yes:D
Changed signature of new to accept Arcsled::Db instead of directory path. So now its up to user to ensure correct lifetime.
@k-u-s If this PR is meant to introduce an example of a sled store, it does not need to be a complete application. Other parts, such as the network, are repeated codes and will be a maintenance burden.
This example is based on one early versions of rocksdb sample (which looks quite similar to mem) both of them have those parts. And to keep them similar as much as possible I only replace store.rs and added test_store.rs, rest should be pretty much without change.
I think its better to have example projects to follow same convention. if you prefer I could modify it but right now I am not sure how would you like it to look. One think that came to my mind is to merge mem/rocksdb/sled examples (if they in fact only differs in store.rs impl) into one where you can change underling store by passing cmd flag or setting some env variable.
This example is based on one early versions of rocksdb sample (which looks quite similar to mem) both of them have those parts. And to keep them similar as much as possible I only replace store.rs and added test_store.rs, rest should be pretty much without change.
I think its better to have example projects to follow same convention. if you prefer I could modify it but right now I am not sure how would you like it to look. One think that came to my mind is to merge mem/rocksdb/sled examples (if they in fact only differs in store.rs impl) into one where you can change underling store by passing cmd flag or setting some env variable.
It'd be better without repeated code. There should be only one application level example. In a previous discussion it's recommended to move the rocksdb store into a standalone crate: https://github.com/datafuselabs/openraft/pull/436#issuecomment-1176001831
After this is done, the example could have a feature to switch to a different storage implementation, e.g.: cargo test --features memstore|rocksdbstore|sledstore.
Took a little longer cause Im evaluating different options since sled seams to not be maintained anymore.
Anyway added sledstore crate similar to rockstore, is this what you had in mind?
Took a little longer cause Im evaluating different options since sled seams to not be maintained anymore.
Anyway added sledstore crate similar to rockstore, is this what you had in mind?
Thank you man!
The overall implementation is ok, except there are some consistency issues.
E.g.: updating to last_applied and applying a log to the state machine has to be in an atomic operation. Otherwise, there is a chance a log entry will be applied twice.
In such a case, a sled::Tree::transaction() has to be used.
And it does not have to be a complete application, since the network and application framework is almost the same as the other two examples.
a sled-store can be just a crate that implements RaftStorage. Similar to the standalone rocksdb-based store:
https://github.com/datafuselabs/openraft/tree/main/rocksstore
@drmingdrmer removed raft-kv-sled example and used mentioned transaction inside apply_to_state_machine method.
Do you prefer for sled-store name for crate instead of sledstore? I initially chosen that name since rocksstore and memstore do not use -
@drmingdrmer removed raft-kv-sled example and used mentioned transaction inside apply_to_state_machine method.
Do you prefer for
sled-storename for crate instead of sledstore? I initially chosen that name since rocksstore and memstore do not use-
Either one is Ok to me:). To be consistent with names, without - would be good.
So i leave the name as it is.
I see you started ci build but my branch was outdated so i merged current main and fixed errors.
Also I unchecked Squash down commits to one or two logical commits which clearly describe the work you've done. since it no longer true. Do you prefer them squashed?
@k-u-s
Some of the commit messages does not follow the style: <type>: blabla.
It's recommended to rebase commits into one before publishing a PR with git rebase, git rebase -i.
A simple command to rebase your branch onto main and squash them into one commit is:
git update-ref refs/heads/my_branch $(echo "commit_message" | git commit-tree my_branch^{tree} -p main)
Replace my_branch and commit_message with actual values.
I see you also use lint so updated with cargo fmt --all also changed test to use tempdir instead of current working directory
~~Not sure why i did not get those errors when i invoked it locally.~~ Ok from what i can see its different tool.
I'll fixed them tomorrow
Do you have some script or cargo command that verify locally all this checks that ci does?
Do you have some script or cargo command that verify locally all this checks that ci does?
There are several commands in Makefile: make fmt, make fix etc.