Re: [Tarantool-patches] [PATCH v2 4/5] raft: introduce split vote detection

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

21.01.2022 02:02, Vladislav Shpilevoy пишет:
>>> +static void
>>> +raft_check_split_vote(struct raft *raft)
>>> +{
>>> +    /* When leader is known, there is no election. Thus no vote to split. */
>>> +    if (raft->leader != 0)
>>> +        return;
>>> +    /* Not a candidate = can't trigger term bump anyway. */
>>> +    if (!raft->is_candidate)
>>> +        return;
>>> +    /*
>>> +     * WAL write in progress means the state is changing. All is rechecked
>>> +     * when it is done.
>>> +     */
>>> +    if (raft->is_write_in_progress)
>>> +        return;
>>> +    if (!raft_has_split_vote(raft))
>>> +        return;
>>> +    assert(raft_ev_is_active(&raft->timer));
>>> +    /*
>>> +     * Could be already detected before. The timeout would be updated by now
>>> +     * then.
>>> +     */
>>> +    if (raft->timer.repeat < raft->election_timeout)
>>> +        return;
>> I don't think you should decrease timer.repeat.
>> This 'vote speedup' is for a single term only.
>>
>> Besides the check below about delay >= remaining is enough
>> to test if split vote detection was already triggered.
> I update timer.repeat as a flag that the split vote was already seen
> during this term. If I drop this check, each next vote after the first
> detection of split vote will lead to potential timeout decrease depending
> on random. I wanted to decrease it only once - when split vote is seen
> first time. Even managed to add a test for it.
>
> The alternative was to store a flag 'has_split_vote' somewhere in struct
> raft, but I didn't want to add it.

Ok, I see now. Thanks for the explanation!

>
>>> +
>>> +    assert(raft->state == RAFT_STATE_FOLLOWER ||
>>> +           raft->state == RAFT_STATE_CANDIDATE);
>>> +    struct ev_loop *loop = raft_loop();
>>> +    struct ev_timer *timer = &raft->timer;
>>> +    double delay = raft_new_random_election_shift(raft);
>>> +    /*
>>> +     * Could be too late to speed up anything - probably the term is almost
>>> +     * over anyway.
>>> +     */
>>> +    double remaining = raft_ev_timer_remaining(loop, timer);
>>> +    if (delay >= remaining)
>>> +        delay = remaining;
>>> +    say_info("RAFT: split vote is discovered - %s, new term in %lf sec",
>>> +         raft_scores_str(raft), delay);
>>> +    raft_ev_timer_stop(loop, timer);
>>> +    raft_ev_timer_set(timer, delay, delay);
>>
>> ...
>>> diff --git a/test/unit/raft_test_utils.h b/test/unit/raft_test_utils.h
>>> index c68dc3b22..2138a829e 100644
>>> --- a/test/unit/raft_test_utils.h
>>> +++ b/test/unit/raft_test_utils.h
>>> @@ -32,6 +32,7 @@
>>>    #include "fakesys/fakeev.h"
>>>    #include "fiber.h"
>>>    #include "raft/raft.h"
>>> +#include "raft/raft_ev.h"
>> Why do you need it here?
> The tests now use raft_ev_is_active() which is defined in this header. I
> decided to add it here instead of unit/raft.c because the idea of
> raft_test_utils.h is, among other things, to do all the boilerplate work
> such as necessary inclusions.

Ok.

>
> New version of the commit after the comment from v1 about new raft
> members:
>
> ====================
>      raft: introduce split vote detection

Thanks for the fixes! LGTM.

-- 
Serge Petrenko