Feature: Add Raft::begin_receiving_snapshot()

[drmingdrmer]

Changelog

Raft::begin_receiving_snapshot() request the state machine to return a
SnapshotData for receiving snapshot from the leader.
Internally it calls RaftStateMachine::begin_receiving_snapshot()

Handling snapshot receiving is moved out of state-machine worker task.
Now it is in implemented outside the RaftCore.
Receiving snapshot could be totally application specific and should not
be part of Openraft.

The in sm-worker snapshot receiving is removed.

• Part of Refactor snapshot API #606

---

This change is 

[schreter]

Reviewed 14 of 18 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @drmingdrmer)

---

openraft/src/core/raft_msg/mod.rs line 77 at r6 (raw file):

    BeginReceivingSnapshot {
        vote: Vote<C::NodeId>,
        tx: ResultSender<Box<SnapshotDataOf<C>>, HigherVote<C::NodeId>>,

Since you are at it, wouldn't it make sense to remove this SnapshotData dichotomy where it's used both for sending the snapshot (reading from the SM) and receiving the snapshot (writing to the SM)?

IMHO it would be clearer to have two separate interfaces. Or am I missing/misunderstanding something here?

---

openraft/src/network/stream_snapshot.rs line 23 at r6 (raw file):

        _req: InstallSnapshotRequest<C>,
    ) -> Result<Option<Snapshot<C>>, StorageError<C::NodeId>> {
        unimplemented!("receive_snapshot is only implemented with SnapshotData with AsyncRead + AsyncSeek ...")

Why AsyncRead? You need AsyncWrite to write to the SM.

---

openraft/src/network/stream_snapshot.rs line 31 at r6 (raw file):

impl<C: RaftTypeConfig> SnapshotTransport<C> for Chunked
where C::SnapshotData: AsyncRead + AsyncWrite + AsyncSeek + OptionalSend + OptionalSync + Unpin + 'static

This is for receiving via AsyncWrite at an offset. So how do I implement it to receive in a different way?

Code quote:

/// Receive snapshot by chunks.
pub(crate) struct Chunked {}

impl<C: RaftTypeConfig> SnapshotTransport<C> for Chunked
where C::SnapshotData: AsyncRead + AsyncWrite + AsyncSeek + OptionalSend + OptionalSync + Unpin + 'static

---

openraft/src/raft/mod.rs line 451 at r6 (raw file):

                }
            };
            *streaming = Some(Streaming::new(req.meta.snapshot_id.clone(), snapshot_data));

I.e., here it's not data of the snapshot, but rather something where to write the snapshot into, right?

Also, regarding the above comment, this still requires the snapshot to implement AsyncWrite + AsyncSeek. So how to stream differently (in 2MB segment chunks in our case)? I know you implemented something for full snapshot install, but I'm missing where the snapshot is taken on the leader for the subsequent out-of-band transport and how to configure it properly (probably I missed it).

---

openraft/src/raft/message/install_snapshot.rs line 43 at r6 (raw file):

impl<C: RaftTypeConfig> MessageSummary<InstallSnapshotRequest<C>> for InstallSnapshotRequest<C> {
    fn summary(&self) -> String {

BTW, wouldn't it make sense to get rid of those summary() stuff in favor of Display and/or Debug (depending on use case)? Alternatively change it to a formatting trait, but not materializing a String - that's so non-Rusty...

[drmingdrmer]

Reviewed 16 of 18 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @schreter)

---

openraft/src/core/raft_msg/mod.rs line 77 at r6 (raw file):

Previously, schreter wrote…

Since you are at it, wouldn't it make sense to remove this SnapshotData dichotomy where it's used both for sending the snapshot (reading from the SM) and receiving the snapshot (writing to the SM)?

IMHO it would be clearer to have two separate interfaces. Or am I missing/misunderstanding something here?

You are right. I found this issue too. But it will introduce too many changes in this PR.

And when sending/receiving snapshot becomes absolutely application defined(in next several PR maybe), it does not need RaftStateMachine::begin_receiving_snapshot() any more.

The application just get the current snapshot with Raft::get_snapshot() and build a customized receiver struct to receive data and finally build a SnapshotData from this application defined receiver.

make sense with this approach? :)

---

openraft/src/network/stream_snapshot.rs line 23 at r6 (raw file):

Previously, schreter wrote…

Why AsyncRead? You need AsyncWrite to write to the SM.

:(

---

openraft/src/network/stream_snapshot.rs line 31 at r6 (raw file):

Previously, schreter wrote…

This is for receiving via AsyncWrite at an offset. So how do I implement it to receive in a different way?

This PR does not intend to introduce any functional changes. But just rewrite it to make it easier for our goal.

This Chunked will be used as a default implementation of snapshot transmission for AsyncRead+AsyncWrite SnapshotData if user application does not want to implement it manually.

---

openraft/src/raft/mod.rs line 451 at r6 (raw file):

Previously, schreter wrote…

I.e., here it's not data of the snapshot, but rather something where to write the snapshot into, right?

Also, regarding the above comment, this still requires the snapshot to implement AsyncWrite + AsyncSeek. So how to stream differently (in 2MB segment chunks in our case)? I know you implemented something for full snapshot install, but I'm missing where the snapshot is taken on the leader for the subsequent out-of-band transport and how to configure it properly (probably I missed it).

In this PR the snapshot sending part is not changed.

Refactoring the sending part will be in next PR. :)
The next PR will still be a refactor PR, which moves the sending part out of ReplicationCore.
Then both of the sending and receiving will be implemented by this Chunked struct, and will be easily replace with other implementation.

---

openraft/src/raft/message/install_snapshot.rs line 43 at r6 (raw file):

Previously, schreter wrote…

BTW, wouldn't it make sense to get rid of those summary() stuff in favor of Display and/or Debug (depending on use case)? Alternatively change it to a formatting trait, but not materializing a String - that's so non-Rusty...

Right...
I'll remove MessageSummary if possible in future development. Thank you for reminding:)

[schreter]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)

---

openraft/src/core/raft_msg/mod.rs line 77 at r6 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

You are right. I found this issue too. But it will introduce too many changes in this PR.

And when sending/receiving snapshot becomes absolutely application defined(in next several PR maybe), it does not need RaftStateMachine::begin_receiving_snapshot() any more.

The application just get the current snapshot with Raft::get_snapshot() and build a customized receiver struct to receive data and finally build a SnapshotData from this application defined receiver.

make sense with this approach? :)

Regarding splitting Snapshot API, sure, not in this PR, that was just a comment :-).

Regarding the snapshot handling, we have two use cases:

• creating a snapshot to truncate the log (where we'll need an application-specific criteria when to do it, e.g., we can configure it via type config)
• creating a snapshot to send over the network to (re)initialize a follower

The first one we could also trigger externally or via an exit into user code every so many log entries. I'm not worried about this one (and we have already implemented it in general).

The second one is the question. Raft drives it and also needs to tell us to take it/send it to the follower/whatever. This is not yet quite clear to me, how it should work.

---

openraft/src/network/stream_snapshot.rs line 31 at r6 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

This PR does not intend to introduce any functional changes. But just rewrite it to make it easier for our goal.

This Chunked will be used as a default implementation of snapshot transmission for AsyncRead+AsyncWrite SnapshotData if user application does not want to implement it manually.

Ack

---

openraft/src/raft/mod.rs line 451 at r6 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

In this PR the snapshot sending part is not changed.

Refactoring the sending part will be in next PR. :)
The next PR will still be a refactor PR, which moves the sending part out of ReplicationCore.
Then both of the sending and receiving will be implemented by this Chunked struct, and will be easily replace with other implementation.

Ack

[drmingdrmer]

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

---

openraft/src/core/raft_msg/mod.rs line 77 at r6 (raw file):

• creating a snapshot to send over the network to (re)initialize a follower
  The second one is the question. Raft drives it and also needs to tell us to take it/send it to the follower/whatever. This is not yet quite clear to me, how it should work.

I'll try to answer your question in next PR:)