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

[Serge Petrenko <sergepetrenko@tarantool.org>]

20.11.2020 02:43, Vladislav Shpilevoy пишет:
> 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.


Haha, ok.


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


Thanks for the fix!

-- 
Serge Petrenko