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 feature flag single-term-leader #1289

Merged
merged 4 commits into from
Jan 3, 2025

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Dec 30, 2024

Changelog

Change: Remove feature flag single-term-leader

In OpenRaft, whether a term supports multiple or single leaders is
determined by the ordering implementation of LeaderId. This behavior
is independent of compilation and can be configured at the application
level, removing the need for a compilation flag single-term-leader.

Changes:

  • The single-term-leader feature flag is removed.
  • Leader mode (multi-leader or single-leader per term) is now controlled
    by the LeaderId associated type in the application configuration.

Configuration Guide:

To enable standard Raft mode (single leader per term):

  • Set LeaderId to openraft::impls::leader_id_std::LeaderId in the
    declare_raft_types! statement, or within the RaftTypeConfig
    implementation.

Example:

openraft::declare_raft_types!(
    pub TypeConfig:
        D = String,
        LeaderId = openraft::impls::leader_id_std::LeaderId<TypeConfig>,
);

To use the default multi-leader per term mode, leave LeaderId
unset or explicitly set it to
openraft::impls::leader_id_adv::LeaderId.

Example:

openraft::declare_raft_types!(
    pub TypeConfig:
        D = String,
);
// Or:
openraft::declare_raft_types!(
    pub TypeConfig:
        D = String,
        LeaderId = openraft::impls::leader_id_adv::LeaderId<TypeConfig>,
);

Upgrade tip:

If the single-term-leader flag was previously used, remove it and
configure LeaderId as follows:

openraft::declare_raft_types!(
    pub TypeConfig:
        LeaderId = openraft::impls::leader_id_std::LeaderId<TypeConfig>,
);
Feature: Abstract LeaderId and CommittedLeaderId

Add LeaderId: RaftLeaderId to RaftTypeConfig to allow customizing leader ID
implementations.


This change is Reviewable

Add `LeaderId: RaftLeaderId` to `RaftTypeConfig` to allow customizing leader ID
implementations.

- Part of databendlabs#1278
In OpenRaft, whether a term supports multiple or single leaders is
determined by the ordering implementation of `LeaderId`. This behavior
is independent of compilation and can be configured at the application
level, removing the need for a compilation flag `single-term-leader`.

### Changes:

- The `single-term-leader` feature flag is removed.
- Leader mode (multi-leader or single-leader per term) is now controlled
  by the `LeaderId` associated type in the application configuration.

### Configuration Guide:

To enable **standard Raft mode (single leader per term)**:
- Set `LeaderId` to `openraft::impls::leader_id_std::LeaderId` in the
  `declare_raft_types!` statement, or within the `RaftTypeConfig`
  implementation.

Example:

```rust
openraft::declare_raft_types!(
    pub TypeConfig:
        D = String,
        LeaderId = openraft::impls::leader_id_std::LeaderId<TypeConfig>,
);
```

To use the **default multi-leader per term mode**, leave `LeaderId`
unset or explicitly set it to
`openraft::impls::leader_id_adv::LeaderId`.

Example:

```rust
openraft::declare_raft_types!(
    pub TypeConfig:
        D = String,
);
// Or:
openraft::declare_raft_types!(
    pub TypeConfig:
        D = String,
        LeaderId = openraft::impls::leader_id_adv::LeaderId<TypeConfig>,
);
```

Upgrade tip:

If the `single-term-leader` flag was previously used, remove it and
configure `LeaderId` as follows:

```rust
openraft::declare_raft_types!(
    pub TypeConfig:
        LeaderId = openraft::impls::leader_id_std::LeaderId<TypeConfig>,
);
```
@drmingdrmer drmingdrmer force-pushed the 179-leader-id branch 4 times, most recently from afe2134 to 91ac452 Compare December 31, 2024 13:59
Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Reviewed 96 of 96 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)


openraft/Cargo.toml line 67 at r1 (raw file):

# This feature is removed.
# Use `openraft::impls::leader_id_std::Leader` for `RaftTypeConfig`
# to enable standard Raft leader election.

I hope we can deprecate features, but looks like it is not something doable today: rust-lang/cargo#7130

Update: I just saw that compile_error!(), which should work for us


openraft/src/docs/feature_flags/feature-flags.md line 28 at r1 (raw file):

**This feature flag is removed**.
User [`leader_id_std::LeaderId`] in [`RaftTypeConfig`] instead.
-User [`leader_id_std::LeaderId`] in [`RaftTypeConfig`] instead.
+Use [`leader_id_std::LeaderId`] in [`RaftTypeConfig`] instead.

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

BTW, should we include this in getting-started.md

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Hmmm, maybe not, considering that the default behavior remains unchanged 🤔

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

- Update the `raft-kv-memstore-grpc` example to use the protobuf-defined `LeaderId`.
- Automatically implement `CommittedLeaderId` for all types.
- Add `openraft::vote::LeaderIdCompare` to provide comparison functions for both single-leader-per-term and multi-leader-per-term implementations.
@drmingdrmer drmingdrmer requested a review from SteveLauC January 2, 2025 07:06
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: all files reviewed, 2 unresolved discussions (waiting on @schreter and @SteveLauC)


openraft/src/docs/feature_flags/feature-flags.md line 28 at r1 (raw file):

Previously, SteveLauC (SteveLauC) wrote…
-User [`leader_id_std::LeaderId`] in [`RaftTypeConfig`] instead.
+Use [`leader_id_std::LeaderId`] in [`RaftTypeConfig`] instead.

Done.

SteveLauC
SteveLauC previously approved these changes Jan 2, 2025
Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 95 of 96 files reviewed, 2 unresolved discussions (waiting on @schreter)

schreter
schreter previously approved these changes Jan 2, 2025
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 95 of 96 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SteveLauC)


openraft/src/vote/leader_id/leader_id_adv.rs line 58 at r2 (raw file):

    }

    fn node_id_ref(&self) -> Option<&C::NodeId> {

Even though it returns a reference, does the _ref suffix make sense? I'd just name it node_id().

@drmingdrmer drmingdrmer requested a review from SteveLauC January 2, 2025 15:25
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: all files reviewed, 2 unresolved discussions (waiting on @SteveLauC)


openraft/src/vote/leader_id/leader_id_adv.rs line 58 at r2 (raw file):

Previously, schreter wrote…

Even though it returns a reference, does the _ref suffix make sense? I'd just name it node_id().

I've been hesitant about choosing between node_id() and node_id_ref(). I also prefer node_id().

However, what if there's another method that returns Option<C::NodeId>? Should we avoid providing two similar methods, such as node_id() -> Option<C::NodeId> and node_id_ref() -> Option<&C::NodeId>? That said, node_id() -> Option<C::NodeId> can be more convenient in certain cases.

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: all files reviewed, 2 unresolved discussions (waiting on @SteveLauC)


openraft/src/vote/leader_id/leader_id_adv.rs line 58 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I've been hesitant about choosing between node_id() and node_id_ref(). I also prefer node_id().

However, what if there's another method that returns Option<C::NodeId>? Should we avoid providing two similar methods, such as node_id() -> Option<C::NodeId> and node_id_ref() -> Option<&C::NodeId>? That said, node_id() -> Option<C::NodeId> can be more convenient in certain cases.

Hm.. Let's keep node_id() -> Option<&C::NodeId> and if needed, to_node_id(&self) -> Option<C::NodeId> looks like a good choice?

Copy link
Collaborator

@SteveLauC SteveLauC 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: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)


openraft/src/vote/leader_id/leader_id_adv.rs line 58 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Hm.. Let's keep node_id() -> Option<&C::NodeId> and if needed, to_node_id(&self) -> Option<C::NodeId> looks like a good choice?

I also prefer the node_id(&self) -> Option<&C::NodeId> option as it conforms to the Rust API guidelines.

Regarding how to return an owned NodeId, I think I would just let the users invoke copied() or cloned() themselves depending on if their NodeID is Copy 🫣

@drmingdrmer drmingdrmer dismissed stale reviews from schreter and SteveLauC via c55c962 January 3, 2025 01:59
@drmingdrmer drmingdrmer merged commit 9bc0aef into databendlabs:main Jan 3, 2025
33 of 34 checks passed
@drmingdrmer drmingdrmer deleted the 179-leader-id branch January 3, 2025 02:23
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.

3 participants