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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.
====================