From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergepetrenko@tarantool.org>
Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109])
 (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 ED2274429E1
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 25 Jun 2020 10:59:17 +0300 (MSK)
References: <923962ad34ef999c7e819b7a84891e204de23646.1592951940.git.v.shpilevoy@tarantool.org>
From: Serge Petrenko <sergepetrenko@tarantool.org>
Message-ID: <cc57e1f9-25ef-e48c-6acf-2f82e975481e@tarantool.org>
Date: Thu, 25 Jun 2020 10:59:16 +0300
MIME-Version: 1.0
In-Reply-To: <923962ad34ef999c7e819b7a84891e204de23646.1592951940.git.v.shpilevoy@tarantool.org>
Content-Type: text/plain; charset="utf-8"; format="flowed"
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Subject: Re: [Tarantool-patches] [PATCH 1/1] [tosquash] test: add a test for
	sync heartbeats
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@dev.tarantool.org

Hi! Thanks for the patch!

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.

Other than that, LGTM.

> +test_run:switch('replica')
> + | ---
> + | - true
> + | ...
> +box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
> + | ---
> + | ...
> +test_run:switch('default')
> + | ---
> + | - true
> + | ...
> +-- Commit something non-sync. So as applier writer fiber would
> +-- flush the pending heartbeat and go to sleep with the new huge
> +-- replication timeout.
> +s = box.schema.create_space('test')
> + | ---
> + | ...
> +pk = s:create_index('pk')
> + | ---
> + | ...
> +s:replace{1}
> + | ---
> + | - [1]
> + | ...
> +-- Now commit something sync. It should return immediately even
> +-- though the replication timeout is huge.
> +box.space.sync:replace{4}
> + | ---
> + | - [4]
> + | ...
> +test_run:switch('replica')
> + | ---
> + | - true
> + | ...
> +box.space.sync:select{4}
> + | ---
> + | - - [4]
> + | ...
> +
>   -- Cleanup.
>   test_run:cmd('switch default')
>    | ---
> @@ -195,6 +242,9 @@ test_run:cmd('delete server replica')
>    | ---
>    | - true
>    | ...
> +box.space.test:drop()
> + | ---
> + | ...
>   box.space.sync:drop()
>    | ---
>    | ...
> diff --git a/test/replication/sync_replication_sanity.test.lua b/test/replication/sync_replication_sanity.test.lua
> index 8715a4600..f769804ca 100644
> --- a/test/replication/sync_replication_sanity.test.lua
> +++ b/test/replication/sync_replication_sanity.test.lua
> @@ -71,11 +71,33 @@ box.space.sync:select{}
>   test_run:cmd('restart server replica')
>   box.space.sync:select{}
>   
> +--
> +-- 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}
> +test_run:switch('replica')
> +box.cfg{replication_timeout = 1000, replication_synchro_timeout = 1000}
> +test_run:switch('default')
> +-- Commit something non-sync. So as applier writer fiber would
> +-- flush the pending heartbeat and go to sleep with the new huge
> +-- replication timeout.
> +s = box.schema.create_space('test')
> +pk = s:create_index('pk')
> +s:replace{1}
> +-- Now commit something sync. It should return immediately even
> +-- though the replication timeout is huge.
> +box.space.sync:replace{4}
> +test_run:switch('replica')
> +box.space.sync:select{4}
> +
>   -- Cleanup.
>   test_run:cmd('switch default')
>   
>   box.cfg{replication_synchro_quorum=quorum, replication_synchro_timeout=timeout}
>   test_run:cmd('stop server replica')
>   test_run:cmd('delete server replica')
> +box.space.test:drop()
>   box.space.sync:drop()
>   box.schema.user.revoke('guest', 'replication')

-- 
Serge Petrenko