Re: [Tarantool-patches] [PATCH 2/2] raft: fix crash on candidate cfg during WAL write

[Serge Petrenko <sergepetrenko@tarantool.org>]

10.11.2020 01:42, Vladislav Shpilevoy пишет:
> Thanks for the review!
>
>>> diff --git a/test/replication/gh-5506-election-on-off.result b/test/replication/gh-5506-election-on-off.result
>>> index 1a718396f..4fbc31986 100644
>>> --- a/test/replication/gh-5506-election-on-off.result
>>> +++ b/test/replication/gh-5506-election-on-off.result
>>> @@ -47,6 +47,91 @@ box.error.injection.set("ERRINJ_WAL_DELAY", false)
>>>     | - ok
>>>     | ...
>>>    +box.cfg{                                                                        \
>>> +    election_mode = old_election_mode,                                          \
>>> +}
>>> + | ---
>>> + | ...
>>> +
>>> +--
>>> +-- Another crash could happen when election mode was configured to be
>>> +-- 'candidate' with a known leader, but there was a not finished WAL write.
>>> +-- The node tried to start waiting for the leader death, even though with an
>>> +-- active WAL write it should wait for its end first.
>>> +--
>>> +box.schema.user.grant('guest', 'super')
>>
>> box.schema.user.grant('guest', 'replication') is enough.
> I know. But I prefer using super, because it works always. I don't
> want to think of the weakest possible rights which would fit. So
> I usually add 'super' and forget about it.

Ok.

>
>>> + | ---
>>> + | ...
>>> +test_run:cmd('create server replica with rpl_master=default,\
>>> +              script="replication/replica.lua"')
>>> + | ---
>>> + | - true
>>> + | ...
>>> +test_run:cmd('start server replica with wait=True, wait_load=True')
>>> + | ---
>>> + | - true
>>> + | ...
>>> +
>>> +test_run:switch('replica')
>>> + | ---
>>> + | - true
>>> + | ...
>>> +box.cfg{election_mode = 'voter'}
>>> + | ---
>>> + | ...
>>> +box.error.injection.set("ERRINJ_WAL_DELAY_COUNTDOWN", 0)
>>> + | ---
>>> + | - ok
>>> + | ...
>>> +
>>> +test_run:switch('default')
>>> + | ---
>>> + | - true
>>> + | ...
>>> +box.cfg{election_mode = 'candidate'}
>>> + | ---
>>> + | ...
>>> +test_run:wait_cond(function()                                                   \
>>> +    return box.info.election.leader ~= 0                                        \
>>> +end)
>>
>> Wouldn't it be simpler to wait for `box.info.election.state == 'leader'`?
> I don't mind.
>
> ====================
> diff --git a/test/replication/gh-5506-election-on-off.result b/test/replication/gh-5506-election-on-off.result
> index 4fbc31986..b8abd7ecd 100644
> --- a/test/replication/gh-5506-election-on-off.result
> +++ b/test/replication/gh-5506-election-on-off.result
> @@ -92,7 +92,7 @@ box.cfg{election_mode = 'candidate'}
>    | ---
>    | ...
>   test_run:wait_cond(function()                                                   \
> -    return box.info.election.leader ~= 0                                        \
> +    return box.info.election.state == 'leader'                                  \
>   end)
>    | ---
>    | - true
> diff --git a/test/replication/gh-5506-election-on-off.test.lua b/test/replication/gh-5506-election-on-off.test.lua
> index bb89477d1..476b00ec0 100644
> --- a/test/replication/gh-5506-election-on-off.test.lua
> +++ b/test/replication/gh-5506-election-on-off.test.lua
> @@ -47,7 +47,7 @@ box.error.injection.set("ERRINJ_WAL_DELAY_COUNTDOWN", 0)
>   test_run:switch('default')
>   box.cfg{election_mode = 'candidate'}
>   test_run:wait_cond(function()                                                   \
> -    return box.info.election.leader ~= 0                                        \
> +    return box.info.election.state == 'leader'                                  \
>   end)
>   
>   test_run:switch('replica')

Thanks for the changes!

LGTM.

-- 
Serge Petrenko