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

Change: remove N, LS, SM from Raft<C, N, LS, SM> #941

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Nov 20, 2023

Changelog

Change: remove N, LS, SM from Raft<C, N, LS, SM>
  • Raft<C, ..>: is a control handle of RaftCore and it does not directly
    rely on N: RaftNetworkFactory, LS: RaftLogStorage and
    SM: RaftStateMachine.
    Thus these types should not be part of Raft.

    In this commit, we remove N, LS, SM from Raft<C, N, LS, SM>,
    RaftInner<C, N, LS> and RaftMsg<C, N, LS>.
    Type N, LS, SM now are only used by Raft::new() which needs these
    types to create RaftCore.

  • Raft::external_request(): Another change is the signature of the
    Fn passed to Raft::external_request() changes from
    FnOnce(&RaftState, &mut LS, &mut N) to FnOnce(&RaftState).

  • Fix: the FnOnce passed to Raft::external_request() should always
    be Send, unoptionally. Because it is used to send from Raft to
    RaftCore.

  • Fix: RaftMsg::ExternalRequest: should not rely on RaftLogStorage and RaftNetworkFactory #939


This change is Reviewable

- `Raft<C, ..>`: is a control handle of `RaftCore` and it does not directly
  rely on `N: RaftNetworkFactory`, `LS: RaftLogStorage` and
  `SM: RaftStateMachine`.
  Thus these types should not be part of `Raft`.

  In this commit, we remove `N, LS, SM` from `Raft<C, N, LS, SM>`,
  `RaftInner<C, N, LS>` and `RaftMsg<C, N, LS>`.
  Type `N, LS, SM` now are only used by `Raft::new()` which needs these
  types to create `RaftCore`.

- `Raft::external_request()`: Another change is the signature of the
  `Fn` passed to `Raft::external_request()` changes from
  `FnOnce(&RaftState, &mut LS, &mut N)` to `FnOnce(&RaftState)`.

- Fix: the `FnOnce` passed to `Raft::external_request()` should always
  be `Send`, unoptionally. Because it is used to send from `Raft` to
  `RaftCore`.

- Fix: databendlabs#939
schreter
schreter previously approved these changes Nov 20, 2023
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 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ariesdevil, @drmingdrmer, and @lichuang)


openraft/src/raft/mod.rs line 145 at r1 (raw file):

// Notably, the state machine, log storage and network factory DO NOT have to be `Send`, those
// are only used within Raft task(s) on a single thread.
unsafe impl<C> Send for Raft<C>

This can then be removed, since it's implicitly Send.


openraft/src/raft/mod.rs line 160 at r1 (raw file):

//
// See above for details.
unsafe impl<C> Sync for Raft<C>

Same here.

@drmingdrmer
Copy link
Member Author

This can then be removed, since it's implicitly Send.

Thank you! I missed this part.

@drmingdrmer drmingdrmer added this pull request to the merge queue Nov 20, 2023
Merged via the queue into databendlabs:main with commit a209c1f Nov 20, 2023
24 checks passed
@drmingdrmer drmingdrmer deleted the 31-type-config branch November 20, 2023 13:25
@drmingdrmer drmingdrmer mentioned this pull request Mar 9, 2024
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.

RaftMsg::ExternalRequest: should not rely on RaftLogStorage and RaftNetworkFactory
3 participants