Tarantool development patches archive
 help / color / mirror / Atom feed
From: Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org,
	yaroslav.dynnikov@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 3/9] test: introduce a helper to wait for bucket GC
Date: Wed, 10 Feb 2021 11:57:56 +0300
Message-ID: <4436bd47-7656-340d-31c3-de4146ab6aa6@tarantool.org> (raw)
In-Reply-To: <b37b547e38c26ad6274cb7b02e41b7e92eca4c3f.1612914070.git.v.shpilevoy@tarantool.org>

Hi! Thanks for your patch! LGTM but I have one question.

Maybe it's reasonable to add some timeout in this function?

AFAIK test-run terminates tests after 120 seconds of inactivity it seems 
too long for such simple case.

But anyway it's up to you.

On 10/02/2021 02:46, Vladislav Shpilevoy wrote:
> In the tests to wait for bucket deletion by GC it was necessary
> to have a long loop expression which checks _bucket space and
> wakes up GC fiber if the bucket is not deleted yet.
>
> Soon the GC wakeup won't be necessary as GC algorithm will become
> reactive instead of proactive.
>
> In order not to remove the wakeup from all places in the main
> patch, and to simplify the waiting the patch introduces a function
> wait_bucket_is_collected().
>
> The reactive GC will delete GC wakeup from this function and all
> the tests still will pass in time.
> ---
>   test/lua_libs/storage_template.lua        | 10 ++++++++++
>   test/rebalancer/bucket_ref.result         |  7 ++-----
>   test/rebalancer/bucket_ref.test.lua       |  5 ++---
>   test/rebalancer/errinj.result             | 13 +++++--------
>   test/rebalancer/errinj.test.lua           |  7 +++----
>   test/rebalancer/rebalancer.result         |  5 +----
>   test/rebalancer/rebalancer.test.lua       |  3 +--
>   test/rebalancer/receiving_bucket.result   |  2 +-
>   test/rebalancer/receiving_bucket.test.lua |  2 +-
>   test/reload_evolution/storage.result      |  5 +----
>   test/reload_evolution/storage.test.lua    |  3 +--
>   11 files changed, 28 insertions(+), 34 deletions(-)
>
> diff --git a/test/lua_libs/storage_template.lua b/test/lua_libs/storage_template.lua
> index 84e4180..21409bd 100644
> --- a/test/lua_libs/storage_template.lua
> +++ b/test/lua_libs/storage_template.lua
> @@ -165,3 +165,13 @@ function wait_rebalancer_state(state, test_run)
>           vshard.storage.rebalancer_wakeup()
>       end
>   end
> +
> +function wait_bucket_is_collected(id)
> +    test_run:wait_cond(function()
> +        if not box.space._bucket:get{id} then
> +            return true
> +        end
> +        vshard.storage.recovery_wakeup()
> +        vshard.storage.garbage_collector_wakeup()
> +    end)
> +end
> diff --git a/test/rebalancer/bucket_ref.result b/test/rebalancer/bucket_ref.result
> index b66e449..b8fc7ff 100644
> --- a/test/rebalancer/bucket_ref.result
> +++ b/test/rebalancer/bucket_ref.result
> @@ -243,7 +243,7 @@ vshard.storage.buckets_info(1)
>       destination: <replicaset_2>
>       id: 1
>   ...
> -while box.space._bucket:get{1} do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end
> +wait_bucket_is_collected(1)
>   ---
>   ...
>   _ = test_run:switch('box_2_a')
> @@ -292,10 +292,7 @@ vshard.storage.buckets_info(1)
>   finish_refs = true
>   ---
>   ...
> -while vshard.storage.buckets_info(1)[1].rw_lock do fiber.sleep(0.01) end
> ----
> -...
> -while box.space._bucket:get{1} do fiber.sleep(0.01) end
> +wait_bucket_is_collected(1)
>   ---
>   ...
>   _ = test_run:switch('box_1_a')
> diff --git a/test/rebalancer/bucket_ref.test.lua b/test/rebalancer/bucket_ref.test.lua
> index 49ba583..213ced3 100644
> --- a/test/rebalancer/bucket_ref.test.lua
> +++ b/test/rebalancer/bucket_ref.test.lua
> @@ -73,7 +73,7 @@ vshard.storage.bucket_refro(1)
>   finish_refs = true
>   while f1:status() ~= 'dead' do fiber.sleep(0.01) end
>   vshard.storage.buckets_info(1)
> -while box.space._bucket:get{1} do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end
> +wait_bucket_is_collected(1)
>   _ = test_run:switch('box_2_a')
>   vshard.storage.buckets_info(1)
>   vshard.storage.internal.errinj.ERRINJ_LONG_RECEIVE = false
> @@ -89,8 +89,7 @@ while not vshard.storage.buckets_info(1)[1].rw_lock do fiber.sleep(0.01) end
>   fiber.sleep(0.2)
>   vshard.storage.buckets_info(1)
>   finish_refs = true
> -while vshard.storage.buckets_info(1)[1].rw_lock do fiber.sleep(0.01) end
> -while box.space._bucket:get{1} do fiber.sleep(0.01) end
> +wait_bucket_is_collected(1)
>   _ = test_run:switch('box_1_a')
>   vshard.storage.buckets_info(1)
>   
> diff --git a/test/rebalancer/errinj.result b/test/rebalancer/errinj.result
> index 214e7d8..e50eb72 100644
> --- a/test/rebalancer/errinj.result
> +++ b/test/rebalancer/errinj.result
> @@ -237,7 +237,10 @@ _bucket:get{36}
>   -- Buckets became 'active' on box_2_a, but still are sending on
>   -- box_1_a. Wait until it is marked as garbage on box_1_a by the
>   -- recovery fiber.
> -while _bucket:get{35} ~= nil or _bucket:get{36} ~= nil do vshard.storage.recovery_wakeup() fiber.sleep(0.001) end
> +wait_bucket_is_collected(35)
> +---
> +...
> +wait_bucket_is_collected(36)
>   ---
>   ...
>   _ = test_run:switch('box_2_a')
> @@ -278,7 +281,7 @@ while not _bucket:get{36} do fiber.sleep(0.0001) end
>   _ = test_run:switch('box_1_a')
>   ---
>   ...
> -while _bucket:get{36} do vshard.storage.recovery_wakeup() vshard.storage.garbage_collector_wakeup() fiber.sleep(0.001) end
> +wait_bucket_is_collected(36)
>   ---
>   ...
>   _bucket:get{36}
> @@ -295,12 +298,6 @@ box.error.injection.set('ERRINJ_WAL_DELAY', false)
>   ---
>   - ok
>   ...
> -_ = test_run:switch('box_1_a')
> ----
> -...
> -while _bucket:get{36} and _bucket:get{36}.status == vshard.consts.BUCKET.ACTIVE do fiber.sleep(0.001) end
> ----
> -...
>   test_run:switch('default')
>   ---
>   - true
> diff --git a/test/rebalancer/errinj.test.lua b/test/rebalancer/errinj.test.lua
> index 66fbe5e..2cc4a69 100644
> --- a/test/rebalancer/errinj.test.lua
> +++ b/test/rebalancer/errinj.test.lua
> @@ -107,7 +107,8 @@ _bucket:get{36}
>   -- Buckets became 'active' on box_2_a, but still are sending on
>   -- box_1_a. Wait until it is marked as garbage on box_1_a by the
>   -- recovery fiber.
> -while _bucket:get{35} ~= nil or _bucket:get{36} ~= nil do vshard.storage.recovery_wakeup() fiber.sleep(0.001) end
> +wait_bucket_is_collected(35)
> +wait_bucket_is_collected(36)
>   _ = test_run:switch('box_2_a')
>   _bucket:get{35}
>   _bucket:get{36}
> @@ -124,13 +125,11 @@ f1 = fiber.create(function() ret1, err1 = vshard.storage.bucket_send(36, util.re
>   _ = test_run:switch('box_2_a')
>   while not _bucket:get{36} do fiber.sleep(0.0001) end
>   _ = test_run:switch('box_1_a')
> -while _bucket:get{36} do vshard.storage.recovery_wakeup() vshard.storage.garbage_collector_wakeup() fiber.sleep(0.001) end
> +wait_bucket_is_collected(36)
>   _bucket:get{36}
>   _ = test_run:switch('box_2_a')
>   _bucket:get{36}
>   box.error.injection.set('ERRINJ_WAL_DELAY', false)
> -_ = test_run:switch('box_1_a')
> -while _bucket:get{36} and _bucket:get{36}.status == vshard.consts.BUCKET.ACTIVE do fiber.sleep(0.001) end
>   
>   test_run:switch('default')
>   test_run:drop_cluster(REPLICASET_2)
> diff --git a/test/rebalancer/rebalancer.result b/test/rebalancer/rebalancer.result
> index 3607e93..098b845 100644
> --- a/test/rebalancer/rebalancer.result
> +++ b/test/rebalancer/rebalancer.result
> @@ -334,10 +334,7 @@ vshard.storage.rebalancer_wakeup()
>   -- Now rebalancer makes a bucket SENT. After it the garbage
>   -- collector cleans it and deletes after a timeout.
>   --
> -while _bucket:get{91}.status ~= vshard.consts.BUCKET.SENT do fiber.sleep(0.01) end
> ----
> -...
> -while _bucket:get{91} ~= nil do fiber.sleep(0.1) end
> +wait_bucket_is_collected(91)
>   ---
>   ...
>   wait_rebalancer_state("The cluster is balanced ok", test_run)
> diff --git a/test/rebalancer/rebalancer.test.lua b/test/rebalancer/rebalancer.test.lua
> index 63e690f..308e66d 100644
> --- a/test/rebalancer/rebalancer.test.lua
> +++ b/test/rebalancer/rebalancer.test.lua
> @@ -162,8 +162,7 @@ vshard.storage.rebalancer_wakeup()
>   -- Now rebalancer makes a bucket SENT. After it the garbage
>   -- collector cleans it and deletes after a timeout.
>   --
> -while _bucket:get{91}.status ~= vshard.consts.BUCKET.SENT do fiber.sleep(0.01) end
> -while _bucket:get{91} ~= nil do fiber.sleep(0.1) end
> +wait_bucket_is_collected(91)
>   wait_rebalancer_state("The cluster is balanced ok", test_run)
>   _bucket.index.status:count({vshard.consts.BUCKET.ACTIVE})
>   _bucket.index.status:min({vshard.consts.BUCKET.ACTIVE})
> diff --git a/test/rebalancer/receiving_bucket.result b/test/rebalancer/receiving_bucket.result
> index db6a67f..7d3612b 100644
> --- a/test/rebalancer/receiving_bucket.result
> +++ b/test/rebalancer/receiving_bucket.result
> @@ -374,7 +374,7 @@ vshard.storage.buckets_info(1)
>       destination: <replicaset_1>
>       id: 1
>   ...
> -while box.space._bucket:get{1} do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end
> +wait_bucket_is_collected(1)
>   ---
>   ...
>   vshard.storage.buckets_info(1)
> diff --git a/test/rebalancer/receiving_bucket.test.lua b/test/rebalancer/receiving_bucket.test.lua
> index 1819cbb..24534b3 100644
> --- a/test/rebalancer/receiving_bucket.test.lua
> +++ b/test/rebalancer/receiving_bucket.test.lua
> @@ -137,7 +137,7 @@ box.space.test3:select{100}
>   _ = test_run:switch('box_2_a')
>   vshard.storage.bucket_send(1, util.replicasets[1], {timeout = 0.3})
>   vshard.storage.buckets_info(1)
> -while box.space._bucket:get{1} do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end
> +wait_bucket_is_collected(1)
>   vshard.storage.buckets_info(1)
>   _ = test_run:switch('box_1_a')
>   box.space._bucket:get{1}
> diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/storage.result
> index 4652c4f..753687f 100644
> --- a/test/reload_evolution/storage.result
> +++ b/test/reload_evolution/storage.result
> @@ -129,10 +129,7 @@ vshard.storage.bucket_send(bucket_id_to_move, util.replicasets[1])
>   ---
>   - true
>   ...
> -vshard.storage.garbage_collector_wakeup()
> ----
> -...
> -while box.space._bucket:get({bucket_id_to_move}) do fiber.sleep(0.01) end
> +wait_bucket_is_collected(bucket_id_to_move)
>   ---
>   ...
>   test_run:switch('storage_1_a')
> diff --git a/test/reload_evolution/storage.test.lua b/test/reload_evolution/storage.test.lua
> index 06f7117..639553e 100644
> --- a/test/reload_evolution/storage.test.lua
> +++ b/test/reload_evolution/storage.test.lua
> @@ -51,8 +51,7 @@ vshard.storage.bucket_force_create(2000)
>   vshard.storage.buckets_info()[2000]
>   vshard.storage.call(bucket_id_to_move, 'read', 'do_select', {42})
>   vshard.storage.bucket_send(bucket_id_to_move, util.replicasets[1])
> -vshard.storage.garbage_collector_wakeup()
> -while box.space._bucket:get({bucket_id_to_move}) do fiber.sleep(0.01) end
> +wait_bucket_is_collected(bucket_id_to_move)
>   test_run:switch('storage_1_a')
>   while box.space._bucket:get{bucket_id_to_move}.status ~= vshard.consts.BUCKET.ACTIVE do vshard.storage.recovery_wakeup() fiber.sleep(0.01) end
>   vshard.storage.bucket_send(bucket_id_to_move, util.replicasets[2])

  reply	other threads:[~2021-02-10  8:58 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09 23:46 [Tarantool-patches] [PATCH 0/9] VShard Map-Reduce, part 1, preparations Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 1/9] rlist: move rlist to a new module Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:57   ` Oleg Babin via Tarantool-patches
2021-02-11  6:50     ` Oleg Babin via Tarantool-patches
2021-02-12  0:09       ` Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 2/9] Use fiber.clock() instead of .time() everywhere Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:57   ` Oleg Babin via Tarantool-patches
2021-02-10 22:33     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 3/9] test: introduce a helper to wait for bucket GC Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:57   ` Oleg Babin via Tarantool-patches [this message]
2021-02-10 22:33     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 4/9] storage: bucket_recv() should check rs lock Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:59   ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 5/9] util: introduce yielding table functions Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:59   ` Oleg Babin via Tarantool-patches
2021-02-10 22:34     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 6/9] cfg: introduce 'deprecated option' feature Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:59   ` Oleg Babin via Tarantool-patches
2021-02-10 22:34     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 7/9] gc: introduce reactive garbage collector Vladislav Shpilevoy via Tarantool-patches
2021-02-10  9:00   ` Oleg Babin via Tarantool-patches
2021-02-10 22:35     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 8/9] recovery: introduce reactive recovery Vladislav Shpilevoy via Tarantool-patches
2021-02-10  9:00   ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 9/9] util: introduce binary heap data structure Vladislav Shpilevoy via Tarantool-patches
2021-02-10  9:01   ` Oleg Babin via Tarantool-patches
2021-02-10 22:36     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:51       ` Oleg Babin via Tarantool-patches
2021-02-12  0:09         ` Vladislav Shpilevoy via Tarantool-patches
2021-03-05 22:03   ` Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:51 ` [Tarantool-patches] [PATCH 0/9] VShard Map-Reduce, part 1, preparations Vladislav Shpilevoy via Tarantool-patches
2021-02-12 11:02   ` Oleg Babin via Tarantool-patches

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=4436bd47-7656-340d-31c3-de4146ab6aa6@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=yaroslav.dynnikov@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git