Refactor: Send heartbeat with dedicated workers

[drmingdrmer]

Changelog

Refactor: Send heartbeat with dedicated workers

Heavy AppendEntries traffic can block heartbeat messages. For example,
future AppendEntries in stream RPC may not receive a response indicating
a follower is alive. In such cases, the leader might time out to extend
its lease, and be considered partitioned from the cluster.

This commit moves heartbeat broadcasting to separate tasks that won't be
blocked by AppendEntries. This ensures the leader can always be
acknowledged with the liveness of followers.

Separate log progress notification and clock progress notification:
When ReplicationCore successfully finished one RPC to Follower/Learner,
it informs the RaftCore to update log progress and clock(heartbeat) progress.
This commit split these two informations into two Notification
variants, in order to make progress handling more clear.

Another improvement is to ignore a heartbeat progress if it is sent with
an older cluster membership config. Because a follower can be removed
and re-added, the obsolete heartbeat progress is invalid. This check is
done by remembering the membership log id in the HeartbeatEvent.

HigherVote can be sent directly to Notification channel.
replication::Response does not need HigherVote variant any more.
And Response is renamed to Progress

---

This change is 

[drmingdrmer]

@ariesdevil

[ariesdevil]

Is id needed here?

[drmingdrmer]

For logging purpose only :(

[schreter]

From what I can say, looks good. Just see the comment - not sure about semantics change.

Reviewed 5 of 16 files at r1, 13 of 13 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ariesdevil and @drmingdrmer)

---

openraft/src/core/heartbeat/worker.rs line 87 at r3 (raw file):

            let payload = AppendEntriesRequest {
                vote: *heartbeat.session_id.leader_vote.deref(),
                prev_log_id: None,

Since it's now sending empty AppendEntriesRequest with prev_log_id == None, this is a bit of a semantics change.

Maybe document on append entries somewhere that the prev_log_id is uninteresting when the entries are empty?

I believe in our implementation we are checking prev_log_id (currently unconditionally).

---

openraft/src/engine/handler/leader_handler/send_heartbeat_test.rs line 60 at r3 (raw file):

        assert_eq!(
            vec![
                //

Why those empty comments (also below)?

[drmingdrmer]

Reviewed 1 of 16 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ariesdevil and @schreter)

---

openraft/src/core/heartbeat/worker.rs line 87 at r3 (raw file):

Previously, schreter wrote…

Since it's now sending empty AppendEntriesRequest with prev_log_id == None, this is a bit of a semantics change.

Maybe document on append entries somewhere that the prev_log_id is uninteresting when the entries are empty?

I believe in our implementation we are checking prev_log_id (currently unconditionally).

Semantically, prev_log_id is used to ensure that the log entries in AppendEntries.entries are consecutive with the entries on the leader by asserting that prev_log_id is equal. Raft ensures that a matching log ID implies all preceding logs match. If entries is empty, it is considered consecutive at any index on the follower, and prev_log_id=None matches the very first position None.

In short AppendEntries with prev_log_id=None && entries.is_empty() means to append nothing at the very beginning, which is always valid.

https://github.com/datafuselabs/openraft/blob/ada27b447a2a89ca97a6ed5061e3cc724883de85/openraft/src/engine/handler/following_handler/mod.rs#L116

---

openraft/src/engine/handler/leader_handler/send_heartbeat_test.rs line 60 at r3 (raw file):

Previously, schreter wrote…

Why those empty comments (also below)?

To force line break and minimize git diff changes :(

[schreter]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ariesdevil)

---

openraft/src/core/heartbeat/worker.rs line 87 at r3 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Semantically, prev_log_id is used to ensure that the log entries in AppendEntries.entries are consecutive with the entries on the leader by asserting that prev_log_id is equal. Raft ensures that a matching log ID implies all preceding logs match. If entries is empty, it is considered consecutive at any index on the follower, and prev_log_id=None matches the very first position None.

In short AppendEntries with prev_log_id=None && entries.is_empty() means to append nothing at the very beginning, which is always valid.

https://github.com/datafuselabs/openraft/blob/ada27b447a2a89ca97a6ed5061e3cc724883de85/openraft/src/engine/handler/following_handler/mod.rs#L116

Strictly speaking, this was previously undefined on the API level. Now it is defined, thanks.

[SteveLauC]

Perhaps we should include this change in the architecture doc?

[drmingdrmer]

Perhaps we should include this change in the architecture doc?

Make sense. I'm gonna do this

[ariesdevil]

And some notes need to be mentioned in the doc. E.g, the node disk may have been damaged, but the heartbeat has been normal.