Re: [Tarantool-patches] [PATCH 09/12] raft: introduce raft_msg, drop xrow dependency

[Serge Petrenko <sergepetrenko@tarantool.org>]

17.11.2020 03:02, Vladislav Shpilevoy пишет:
> Raft used to depend on xrow, because it used raft_request as a
> communication and persistence unit. Xrow is a part of src/box
> library set, so it blocked Raft extraction into src/lib/raft.
>
> This patch makes Raft not depend on xrow. For that Raft introduces
> a new communication and persistence unit - struct raft_msg.
> Interestingly, throughout its source code Raft already uses term
> 'message' to describe requests, so this patch also restores the
> consistency. This is because raft_request name was used to be
> consistent with other *_request structs in xrow.h. Now Raft does
> not depend on this, and can use its own name.
>
> Struct raft_msg repeats raft_request literally, but it actually
> makes sense. Because when Raft is extracted to a new library, it
> may start evolving independently. Its raft_msg may be populated
> with new members, or their behaviour may change depending on how
> the algorithm will evolve.
>
> But inside box it will be possible to tweak and extend raft_msg
> whenever it is necessary, via struct raft_request, and without
> changing the basic library.
>
> For instance, in future we may want to make nodes forward the
> messages to each other during voting to speed the process up, and
> for that we may want to add an explicit 'source' field to
> raft_request, while it won't be necessary on the level of
> raft_msg.
>
> There is a new compatibility layer in src/box/raft.h which hides
> raft_msg details from other box code, and does the msg <-> request
> conversions.
>
> Part of #5303
> ---
>   src/box/applier.cc     |  2 +-
>   src/box/box.cc         |  4 +--
>   src/box/memtx_engine.c |  4 +--
>   src/box/raft.c         | 70 ++++++++++++++++++++++++++++++++++++++----
>   src/box/raft.h         | 24 +++++++++++++++
>   src/box/raftlib.c      | 24 ++++++---------
>   src/box/raftlib.h      | 38 ++++++++++++++++++-----
>   src/box/xrow.h         |  4 +++
>   8 files changed, 137 insertions(+), 33 deletions(-)
>
>
> diff --git a/src/box/raft.c b/src/box/raft.c
> index 845525660..f3652bbcb 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -49,6 +49,28 @@ struct raft box_raft_global = {
>    */
>   static struct trigger box_raft_on_update;
>   
> +static void
> +box_raft_msg_to_request(const struct raft_msg *msg, struct raft_request *req)
> +{
> +	*req = (struct raft_request) {
> +		.term = msg->term,
> +		.vote = msg->vote,
> +		.state = msg->state,
> +		.vclock = msg->vclock,
> +	};
> +}
> +
> +static void
> +box_raft_request_to_msg(const struct raft_request *req, struct raft_msg *msg)
> +{
> +	*msg = (struct raft_msg) {
> +		.term = req->term,
> +		.vote = req->vote,
> +		.state = req->state,
> +		.vclock = req->vclock,
> +	};
> +}
> +
>   static int
>   box_raft_on_update_f(struct trigger *trigger, void *event)
>   {

Have you considered making `struct raft_msg` a member of `struct 
raft_request`?
This way you'll avoid copying.
Yes, xrow will start depending on raftlib, but is this too bad?

Never mind, it'll only work on raft_request -> raft_msg transition,
but not vice versa. So, just an idea.

> diff --git a/src/box/raft.h b/src/box/raft.h
> index 09297273f..4dffce380 100644
> --- a/src/box/raft.h
> +++ b/src/box/raft.h
> @@ -35,6 +35,8 @@
>   extern "C" {
>   #endif
>   
> +struct raft_request;
> +
>   /** Raft state of this instance. */
>   static inline struct raft *
>   box_raft(void)
> @@ -56,6 +58,28 @@ box_raft(void)
>   void
>   box_raft_reconsider_election_quorum(void);
>   
> +/**
> + * Recovery a single Raft request. Raft state machine is not turned on yet, this
> + * works only during instance recovery from the journal.
> + */


Typo: Recovery -> Recover


> +void
> +box_raft_recover(const struct raft_request *req);
> +
> +/** Save complete Raft state into a request to be persisted on disk locally. */
> +void
> +box_raft_checkpoint_local(struct raft_request *req);
> +
> +/**
> + * Save complete Raft state into a request to be sent to other instances of the
> + * cluster.
> + */
> +void
> +box_raft_checkpoint_remote(struct raft_request *req);
> +
> +/** Handle a single Raft request from a node with instance id @a source. */
> +int
> +box_raft_process(struct raft_request *req, uint32_t source);
> +
>   void
>   box_raft_init(void);
>   

-- 
Serge Petrenko