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.

Sign up for GitHub

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

Jump to bottom

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

Closed
drmingdrmer opened this issue Nov 19, 2023 · 3 comments · Fixed by #941
Closed

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

drmingdrmer opened this issue Nov 19, 2023 · 3 comments · Fixed by #941

Comments

@drmingdrmer
Copy link
Member

drmingdrmer commented Nov 19, 2023 •
edited
Loading

ExternalRequest passes the implementation of RaftLogStore and
RaftNetworkFactory to a user defined fn. This adds unnecessary
dependency(N, LS) to RaftMsg, RaftInner and Raft.

enum RaftMsg<C, N, LS> {
    ExternalRequest {
        req: Box< dyn FnOnce(&RaftState<...>, &mut LS, &mut N) >,
    },
}

Update: 2023-11-20:
These types should be erased by replacing them with &dny RaftLogStore and
&dyn RaftNetworkFactory.

Just remove LS and N from ExternalRequest.
Because RaftLogStorage will be moved out of RaftCore task, it's impossible to access &RaftState and &LS at the same time.
@github-actions GitHub Actions
Copy link

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@schreter
Copy link
Collaborator

Thanks, as discussed on Discord, fine for me.

BTW, unless there are other users who feel otherwise, feel free to reduce the parameters to only LS or SM (probably SM makes more sense here, not sure). We can cope with either one in our project.

@drmingdrmer
Copy link
Member Author

dyn RaftLogStorage<C> does not work, because it has an associated type type LogReader: RaftLogReader<C>.
To use it as trait object, the associated type must be specified. But as a parameter of a function signature, the associated type is unknown.

@drmingdrmer drmingdrmer changed the title RaftMsg::ExternalRequest uses dyn T instead of concrete type RaftMsg::ExternalRequest: should not rely on RaftLogStorage and RaftNetworkFactory Nov 20, 2023
@drmingdrmer drmingdrmer mentioned this issue Nov 20, 2023
Closed
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Nov 20, 2023
@drmingdrmer
Change: remove N, LS from Raft<C, N, LS, _>
d929e5b 
- `Raft<C, ..>`: is a control handle of `RaftCore` and it does not directly
  rely on `N: RaftNetworkFactory` and `LS: RaftLogStorage`.
  Thus these two type should not be part of `Raft`.

  In this commit, we remove `N, LS` from `Raft<C, N, LS, SM>`,
  `RaftInner<C, N, LS>` and `RaftMsg<C, N, LS>`.
  Type `N, LS` now is only used by `Raft::new()` which needs these two
  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
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Nov 20, 2023
@drmingdrmer
Change: remove N, LS from Raft<C, N, LS, _>
a034382 
- `Raft<C, ..>`: is a control handle of `RaftCore` and it does not directly
  rely on `N: RaftNetworkFactory` and `LS: RaftLogStorage`.
  Thus these two type should not be part of `Raft`.

  In this commit, we remove `N, LS` from `Raft<C, N, LS, SM>`,
  `RaftInner<C, N, LS>` and `RaftMsg<C, N, LS>`.
  Type `N, LS` now is only used by `Raft::new()` which needs these two
  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
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Nov 20, 2023
@drmingdrmer
Change: remove N, LS, SM from Raft<C, N, LS, SM>
182fe92 
- `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
@drmingdrmer drmingdrmer mentioned this issue Nov 20, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue Nov 20, 2023
@drmingdrmer
Change: remove N, LS, SM from Raft<C, N, LS, SM>
87c0d74 
- `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: #939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@drmingdrmer @schreter