From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id ED8B74696C0 for ; Fri, 20 Nov 2020 02:43:48 +0300 (MSK) From: Vladislav Shpilevoy References: Message-ID: <52e1d00a-1f4c-b750-d72f-2c9256085436@tarantool.org> Date: Fri, 20 Nov 2020 00:43:47 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 09/12] raft: introduce raft_msg, drop xrow dependency List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com Thanks for the review! >> 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? This is what I was trying to avoid. Currently xrow does not depend on anything. It is a pure protocol library. I am not saying it is good, but I don't want to create a precedent here, allowing to link xrow with everything else in future. If it was up to me, I would split xrow, so as each library and subsystem decides how to encode its shit. > 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 Indeed, fixed. ==================== /** - * Recovery a single Raft request. Raft state machine is not turned on yet, this + * Recover a single Raft request. Raft state machine is not turned on yet, this * works only during instance recovery from the journal. ====================