From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 EBC5C41C5DC for ; Thu, 25 Jun 2020 23:58:16 +0300 (MSK) References: <923962ad34ef999c7e819b7a84891e204de23646.1592951940.git.v.shpilevoy@tarantool.org> From: Vladislav Shpilevoy Message-ID: <02623e30-e231-fa7a-ff1d-d557b63aee79@tarantool.org> Date: Thu, 25 Jun 2020 22:58:14 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Serge Petrenko , tarantool-patches@dev.tarantool.org 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. 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()