Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Sergei Voronezhskii <sergw@tarantool.org>
Cc: tarantool-patches@freelists.org,
	Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v2 2/5] test: errinj for pause relay_send
Date: Sun, 21 Oct 2018 23:41:22 +0300	[thread overview]
Message-ID: <20181021204121.c7vucdv4nhblodwy@tkn_work_nb> (raw)
In-Reply-To: <20181019161721.49560-3-sergw@tarantool.org>

Hi!

I don't have objections in general. Some minor comments are below.

Please, answer with fixes, don't just send the whole patch.

WBR, Alexander Turenko.

> Instead of using timeout we need just pause `relay_send`. Can't rely
> on timeout because of various system load in parallel mode. Add new
> errinj which checks boolean in loop and until it is not `True` do not
> pass the method `relay_send` to the next statement.
> 
> To check the read-only mode, need to make a modification of tuple. It
> is enough to call `replace` method. Instead of `delete` and then
> useless verification that we have not delete space by using `get`
> method.
> 

delete space -> delete tuple?

> +-- In the next two cases we try to replace a tuple while replica
> +-- is catching up with the master (local delete, remote delete)

delete -> replace

> +-- case

Nit: period at the end.

> --- check sync
> -errinj.set("ERRINJ_RELAY_TIMEOUT", 0)
> +-- Resume replicaton.

replicaton -> replication

> diff --git a/test/replication/gc.test.lua b/test/replication/gc.test.lua
> index 5100378b3..22921289d 100644
> --- a/test/replication/gc.test.lua
> +++ b/test/replication/gc.test.lua
> @@ -12,6 +12,7 @@ default_checkpoint_count = box.cfg.checkpoint_count
>  box.cfg{checkpoint_count = 1}
>  
>  function wait_gc(n) while #box.info.gc().checkpoints > n do fiber.sleep(0.01) end end
> +function wait_xlog(n, timeout) timeout = timeout or 1.0 return test_run:wait_cond(function() return #fio.glob('./master/*.xlog') == n end, timeout) end
>

Use 'set delimiter' and write it in several lines. Also, below I
proposed to support 'n' being a table to allow count of files being one
of several values. You can use auxiliary function like the following and
type(n) == 'table' check.

function value_in(val, arr)
    for _, elem in ipairs(arr) do
        if val == elem then
            return true
        end
    end
    return false
end

> @@ -31,7 +32,7 @@ for i = 1, 100 do s:auto_increment{} end
>  
>  -- Make sure replica join will take long enough for us to
>  -- invoke garbage collection.
> -box.error.injection.set("ERRINJ_RELAY_TIMEOUT", 0.05)
> +box.error.injection.set("ERRINJ_RELAY_SEND_DELAY", true)
>  
>  -- While the replica is receiving the initial data set,
>  -- make a snapshot and invoke garbage collection, then
> @@ -41,7 +42,7 @@ test_run:cmd("setopt delimiter ';'")
>  fiber.create(function()
>      fiber.sleep(0.1)
>      box.snapshot()
> -    box.error.injection.set("ERRINJ_RELAY_TIMEOUT", 0)
> +    box.error.injection.set("ERRINJ_RELAY_SEND_DELAY", false)
>  end)
>  test_run:cmd("setopt delimiter ''");
>  

The entire comment:

> -- While the replica is receiving the initial data set,
> -- make a snapshot and invoke garbage collection, then
> -- remove the timeout injection so that we don't have to
> -- wait too long for the replica to start.

Proposed: then remove delay to allow replica to start.

> --- Remove the timeout injection so that the replica catches
> +-- Resume replicaton so that the replica catches

replicaton -> replication

> @@ -146,17 +147,16 @@ box.snapshot()
>  _ = s:auto_increment{}
>  box.snapshot()
>  #box.info.gc().checkpoints == 1 or box.info.gc()
> -xlog_count = #fio.glob('./master/*.xlog')
>  -- the replica may have managed to download all data
>  -- from xlog #1 before it was stopped, in which case
>  -- it's OK to collect xlog #1
> -xlog_count == 3 or xlog_count == 2 or fio.listdir('./master')
> +wait_xlog(3, 0.1) or wait_xlog(2, 0.1) or fio.listdir('./master')

You are set timeout to 1.0 for other cases, but 0.2 here. So, 0.2 is
enough? It is better to allow the function to accept a table like {2, 3}
as files count. Use 'set delimiter' and update the function.

  reply	other threads:[~2018-10-21 20:41 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-19 16:17 [PATCH v2 0/5] test: replication parallel mode on Sergei Voronezhskii
2018-10-19 16:17 ` [PATCH v2 1/5] test: cleanup replication tests Sergei Voronezhskii
2018-10-21 20:41   ` Alexander Turenko
2018-10-22  8:07     ` Re[2]: " Sergei Voronezhskii
2018-10-23  3:21       ` Alexander Turenko
2018-10-19 16:17 ` [PATCH v2 2/5] test: errinj for pause relay_send Sergei Voronezhskii
2018-10-21 20:41   ` Alexander Turenko [this message]
2018-10-22  8:42     ` Re[2]: " Sergei Voronezhskii
2018-10-23  3:22       ` Alexander Turenko
2018-10-19 16:17 ` [PATCH v2 3/5] test: put require in proper places Sergei Voronezhskii
2018-10-21 20:41   ` Alexander Turenko
2018-10-19 16:17 ` [PATCH v2 4/5] test: use wait_cond to check follow status Sergei Voronezhskii
2018-10-19 23:24   ` Alexander Turenko
2018-10-25 16:43     ` [tarantool-patches] " Sergei Voronezhskii
2018-10-29 10:41       ` Alexander Turenko
2018-10-31 21:38         ` Re[2]: " Sergei Voronezhskii
2018-10-19 16:17 ` [PATCH v2 5/5] test: replication parallel mode on Sergei Voronezhskii
2018-10-19 23:37 ` [PATCH v2 0/5] " Alexander Turenko
2018-10-19 23:44   ` Alexander Turenko
2018-10-26 12:41     ` Re[2]: " Sergei Voronezhskii
2018-10-26 12:44     ` Sergei Voronezhskii
2018-10-30 17:38       ` Re[3]: " Sergei Voronezhskii
2018-10-31 18:28         ` Alexander Turenko
2018-11-26 13:04           ` Re[2]: " Sergei Voronezhskii
2018-12-05  4:44             ` Re[3]: " Sergei Voronezhskii

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181021204121.c7vucdv4nhblodwy@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=sergw@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH v2 2/5] test: errinj for pause relay_send' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox