From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1BFFC42EF5C for ; Fri, 26 Jun 2020 13:45:09 +0300 (MSK) References: <923962ad34ef999c7e819b7a84891e204de23646.1592951940.git.v.shpilevoy@tarantool.org> <02623e30-e231-fa7a-ff1d-d557b63aee79@tarantool.org> From: Serge Petrenko Message-ID: <78a529d2-eed6-438e-cf67-a24c36c0fe27@tarantool.org> Date: Fri, 26 Jun 2020 13:45:08 +0300 MIME-Version: 1.0 In-Reply-To: <02623e30-e231-fa7a-ff1d-d557b63aee79@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 1/1] [tosquash] test: add a test for sync heartbeats List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.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