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

Test: add example of singlethreaded kv-store #989

Merged
merged 2 commits into from
Jan 14, 2024

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Jan 12, 2024

Changelog

Test: add example of singlethreaded kv-store

This example examples/raft-kv-memstore-singlethreaded illustrates
building a single threaded Openraft application.

By enabling feature flag singlethreaded, Openraft does not require the
data types to be Send, which enables using Rc in place of Arc to
gain a performance improvement.


This change is Reviewable

@drmingdrmer
Copy link
Member Author

@wvwwvwwv @schreter
Please help on reviewing this example of a single-threaded application to determine if it meets the requirements of your environment.

@drmingdrmer drmingdrmer force-pushed the 37-singlethreaded-test branch 3 times, most recently from 9abee80 to 65e3284 Compare January 12, 2024 08:03
This example `examples/raft-kv-memstore-singlethreaded` illustrates
building a single threaded Openraft application.

By enabling feature flag `singlethreaded`, Openraft does not require the
data types to be `Send`, which enables using `Rc` in place of `Arc` to
gain a performance improvement.
@drmingdrmer drmingdrmer force-pushed the 37-singlethreaded-test branch from 65e3284 to 936e70b Compare January 12, 2024 08:12
schreter
schreter previously approved these changes Jan 13, 2024
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

On the first look, it looks good, thanks. I didn't do a very thorough review, though, since I suppose it's mostly copy&paste from another example anyway :-).

Some minor comments below.

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ariesdevil, @drmingdrmer, and @lichuang)


examples/raft-kv-memstore-singlethreaded/src/network.rs line 25 at r1 (raw file):

// NOTE: This could be implemented also on `Arc<ExampleNetwork>`, but since it's empty, implemented
// directly.

?

Suggestion:

// NOTE: This could be implemented also on `Rc<ExampleNetwork>`, but since it's empty, implemented
// directly.

examples/raft-kv-memstore-singlethreaded/src/router.rs line 17 at r1 (raw file):

#[derive(Default)]
pub struct Router {
    pub targets: Rc<Mutex<BTreeMap<NodeId, RequestTx>>>,

Since it's all running in a single thread, why the lock? A RefCell or so would be sufficient, right?


examples/raft-kv-memstore-singlethreaded/src/store.rs line 106 at r1 (raw file):

    pub state_machine: RwLock<StateMachineData>,

    snapshot_idx: Arc<Mutex<u64>>,

Same here - a Rc<Cell<u64>> or so should be sufficient, right?


examples/raft-kv-memstore-singlethreaded/tests/cluster/test_cluster.rs line 27 at r1 (raw file):

    let backtrace = {
        format!("{:?}", Backtrace::force_capture())
        // #[cfg(feature = "bt")]

Why the commented-out code?

Copy link
Member Author

@drmingdrmer drmingdrmer 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. The following fixup commit replaced these multi-threaded primitives.

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ariesdevil and @lichuang)


examples/raft-kv-memstore-singlethreaded/tests/cluster/test_cluster.rs line 27 at r1 (raw file):

Previously, schreter wrote…

Why the commented-out code?

Not used codes. Removed:(

@drmingdrmer drmingdrmer merged commit 5447387 into databendlabs:main Jan 14, 2024
27 checks passed
@drmingdrmer drmingdrmer deleted the 37-singlethreaded-test branch January 14, 2024 04:25
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


examples/raft-kv-memstore-singlethreaded/src/store.rs line 104 at r2 (raw file):

    pub state_machine: RefCell<StateMachineData>,

    snapshot_idx: RefCell<u64>,

BTW, any Copy values can be also stored as Cell, which incurs less overhead (especially, no need for runtime checking). Then, you can use simple get()/set() interface to read/write the cell.

Copy link
Member Author

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


examples/raft-kv-memstore-singlethreaded/src/store.rs line 104 at r2 (raw file):

Previously, schreter wrote…

BTW, any Copy values can be also stored as Cell, which incurs less overhead (especially, no need for runtime checking). Then, you can use simple get()/set() interface to read/write the cell.

Thank you for the suggestion.

I have given thought to employing Cell in the implementation. However, for the sake of simplicity in this example application, I'm aiming to minimize the number of types used. :)

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