Re: [Tarantool-patches] [PATCH 1/1] [tosquash] test: add a test for sync heartbeats

[Serge Petrenko <sergepetrenko@tarantool.org>]

25.06.2020 23:58, Vladislav Shpilevoy пишет:
> Hi! Thanks for the review!
>
>> Please see 1 comment below.
>>
>> 24.06.2020 01:39, Vladislav Shpilevoy пишет:
>>> Should be squashed into the commit closing 5100.
>>> ---
>>> Branch: http://github.com/tarantool/tarantool/tree/gh-4842-sync-replication
>>> Issue: https://github.com/tarantool/tarantool/issues/4842
>>>
>>>    .../sync_replication_sanity.result            | 50 +++++++++++++++++++
>>>    .../sync_replication_sanity.test.lua          | 22 ++++++++
>>>    2 files changed, 72 insertions(+)
>>>
>>> diff --git a/test/replication/sync_replication_sanity.result b/test/replication/sync_replication_sanity.result
>>> index 4b9823d77..a0591dcf3 100644
>>> --- a/test/replication/sync_replication_sanity.result
>>> +++ b/test/replication/sync_replication_sanity.result
>>> @@ -178,6 +178,53 @@ box.space.sync:select{}
>>>     |   - [3]
>>>     | ...
>>>    +--
>>> +-- gh-5100: replica should send ACKs for sync transactions after
>>> +-- WAL write immediately, not waiting for replication timeout or
>>> +-- a CONFIRM.
>>> +--
>>> +box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
>>> + | ---
>>> + | ...
>> You should remember previous replication_timeout here and set it back during cleanup.
> Oh shit, you are right. I looked at how you restore replication_synchro_timeout
> on line 98 and somewhy decided that the normal timeout is also restored.
>
> I also found that instead of configuring master and replica I configured the
> replica 2 times. Because I thought that the previous tests ends in 'default'
> instance. Fixed this too.

LGTM

>
> Force pushed to this commit.
>
> ====================
> diff --git a/test/replication/sync_replication_sanity.result b/test/replication/sync_replication_sanity.result
> index a0591dcf3..8b37ba6f5 100644
> --- a/test/replication/sync_replication_sanity.result
> +++ b/test/replication/sync_replication_sanity.result
> @@ -90,10 +90,10 @@ box.schema.user.grant('guest', 'replication')
>    | ---
>    | ...
>   -- Set up synchronous replication options.
> -quorum = box.cfg.replication_synchro_quorum
> +old_synchro_quorum = box.cfg.replication_synchro_quorum
>    | ---
>    | ...
> -timeout = box.cfg.replication_synchro_timeout
> +old_synchro_timeout = box.cfg.replication_synchro_timeout
>    | ---
>    | ...
>   box.cfg{replication_synchro_quorum=2, replication_synchro_timeout=0.1}
> @@ -186,16 +186,15 @@ box.space.sync:select{}
>   box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
>    | ---
>    | ...
> -test_run:switch('replica')
> +test_run:switch('default')
>    | ---
>    | - true
>    | ...
> -box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
> +old_timeout = box.cfg.replication_timeout
>    | ---
>    | ...
> -test_run:switch('default')
> +box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
>    | ---
> - | - true
>    | ...
>   -- Commit something non-sync. So as applier writer fiber would
>   -- flush the pending heartbeat and go to sleep with the new huge
> @@ -231,7 +230,11 @@ test_run:cmd('switch default')
>    | - true
>    | ...
>   
> -box.cfg{replication_synchro_quorum=quorum, replication_synchro_timeout=timeout}
> +box.cfg{                                                                        \
> +    replication_synchro_quorum = old_synchro_quorum,                            \
> +    replication_synchro_timeout = old_synchro_timeout,                          \
> +    replication_timeout = old_timeout,                                          \
> +}
>    | ---
>    | ...
>   test_run:cmd('stop server replica')
> diff --git a/test/replication/sync_replication_sanity.test.lua b/test/replication/sync_replication_sanity.test.lua
> index f769804ca..b0326fd4b 100644
> --- a/test/replication/sync_replication_sanity.test.lua
> +++ b/test/replication/sync_replication_sanity.test.lua
> @@ -38,8 +38,8 @@ engine = test_run:get_cfg('engine')
>   
>   box.schema.user.grant('guest', 'replication')
>   -- Set up synchronous replication options.
> -quorum = box.cfg.replication_synchro_quorum
> -timeout = box.cfg.replication_synchro_timeout
> +old_synchro_quorum = box.cfg.replication_synchro_quorum
> +old_synchro_timeout = box.cfg.replication_synchro_timeout
>   box.cfg{replication_synchro_quorum=2, replication_synchro_timeout=0.1}
>   
>   test_run:cmd('create server replica with rpl_master=default,\
> @@ -77,9 +77,9 @@ box.space.sync:select{}
>   -- a CONFIRM.
>   --
>   box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
> -test_run:switch('replica')
> -box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
>   test_run:switch('default')
> +old_timeout = box.cfg.replication_timeout
> +box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
>   -- Commit something non-sync. So as applier writer fiber would
>   -- flush the pending heartbeat and go to sleep with the new huge
>   -- replication timeout.
> @@ -95,7 +95,11 @@ box.space.sync:select{4}
>   -- Cleanup.
>   test_run:cmd('switch default')
>   
> -box.cfg{replication_synchro_quorum=quorum, replication_synchro_timeout=timeout}
> +box.cfg{                                                                        \
> +    replication_synchro_quorum = old_synchro_quorum,                            \
> +    replication_synchro_timeout = old_synchro_timeout,                          \
> +    replication_timeout = old_timeout,                                          \
> +}
>   test_run:cmd('stop server replica')
>   test_run:cmd('delete server replica')
>   box.space.test:drop()

-- 
Serge Petrenko