[tarantool-patches] Re: [PATCH 3/3] Introduce destroy module feature
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Jun 23 01:11:55 MSK 2018
Thanks for the patch! See 9 comments below.
On 23/06/2018 00:43, AKhatskevich wrote:
> Introduce functions:
> * vshard.router.destroy()
> * vshard.storage.destroy()
>
> Those functions:
> * close connections
> * stop background fibers
> * delete vshard spaces
> * delete vshard funcitons
> * delete `once` metadate
>
> Closes #121
> ---
> test/lua_libs/util.lua | 34 +++++++++++
> test/router/destroy.result | 108 ++++++++++++++++++++++++++++++++++
> test/router/destroy.test.lua | 39 +++++++++++++
> test/storage/destroy.result | 132 ++++++++++++++++++++++++++++++++++++++++++
> test/storage/destroy.test.lua | 51 ++++++++++++++++
> vshard/router/init.lua | 22 +++++++
> vshard/storage/init.lua | 70 +++++++++++++++++-----
> 7 files changed, 440 insertions(+), 16 deletions(-)
> create mode 100644 test/router/destroy.result
> create mode 100644 test/router/destroy.test.lua
> create mode 100644 test/storage/destroy.result
> create mode 100644 test/storage/destroy.test.lua
>
> diff --git a/test/lua_libs/util.lua b/test/lua_libs/util.lua
> index f2d3b48..ce0ea67 100644
> --- a/test/lua_libs/util.lua
> +++ b/test/lua_libs/util.lua
> @@ -69,9 +69,43 @@ local function wait_master(test_run, replicaset, master)
> log.info('Slaves are connected to a master "%s"', master)
> end
>
> +function vshard_fiber_list()
> + -- Flush jit traces to prevent them from
> + -- keeping its upvalues in memory.
> + jit.flush()
> + collectgarbage()
> + -- Give a fiber time to clean itself.
> + fiber.sleep(0.05)
1. Why do you need this sleep? As far as I know
collectgarbage just cleans all the garbage in the same
fiber with no yields or async tasks.
> + local fibers = fiber.info()
> + local non_vshard_patterns = {
> + '^console',
> + 'feedback_daemon$',
> + '^checkpoint_daemon$',
> + '^main$',
> + '^memtx%.gc$',
> + '^vinyl%.scheduler$',
> + }
> + local names = {}
> + for _, fib in pairs(fibers) do
> + local add = true
> + for _, pattern in pairs(non_vshard_patterns) do
> + if fib.name:match(pattern) then
> + add = false
> + break
> + end
> + end
> + if add then
> + table.insert(names, fib.name)
> + end
> + end
> + table.sort(names)
2. Sort of an array just does nothing, it is not?
> + return names
> +end;
> +
> return {
> check_error = check_error,
> shuffle_masters = shuffle_masters,
> collect_timeouts = collect_timeouts,
> wait_master = wait_master,
> + vshard_fiber_list = vshard_fiber_list,
> }
> diff --git a/test/router/destroy.result b/test/router/destroy.result
> new file mode 100644
> index 0000000..48ba661
> --- /dev/null
> +++ b/test/router/destroy.result
> @@ -0,0 +1,108 @@
> +test_run = require('test_run').new()
> +---
> +...
> +netbox = require('net.box')
> +---
> +...
> +fiber = require('fiber')
> +---
> +...
> +REPLICASET_1 = { 'storage_1_a', 'storage_1_b' }
> +---
> +...
> +REPLICASET_2 = { 'storage_2_a', 'storage_2_b' }
> +---
> +...
> +test_run:create_cluster(REPLICASET_1, 'storage')
> +---
> +...
> +test_run:create_cluster(REPLICASET_2, 'storage')
> +---
> +...
> +util = require('util')
> +---
> +...
> +util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
> +---
> +...
> +util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
> +---
> +...
> +test_run:cmd("create server router_1 with script='router/router_1.lua'")
> +---
> +- true
> +...
> +test_run:cmd("start server router_1")
> +---
> +- true
> +...
> +_ = test_run:cmd("switch router_1")
> +---
> +...
> +util = require('util')
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function wait_fibers_exit()
> + while #util.vshard_fiber_list() > 0 do
> + sleep(0.05)
3. Maybe fiber.sleep? Process sleep looks weird here.
> + end
> +end;
> +---> diff --git a/test/storage/destroy.result b/test/storage/destroy.result
> new file mode 100644
> index 0000000..23ec6bd
> --- /dev/null
> +++ b/test/storage/destroy.result
> @@ -0,0 +1,132 @@
> +test_run = require('test_run').new()
> +---
> +...
> +REPLICASET_1 = { 'storage_1_a', 'storage_1_b' }
> +---
> +...
> +REPLICASET_2 = { 'storage_2_a', 'storage_2_b' }
> +---
> +...
> +test_run:create_cluster(REPLICASET_1, 'storage')
> +---
> +...
> +test_run:create_cluster(REPLICASET_2, 'storage')
> +---
> +...
> +util = require('util')
> +---
> +...
> +util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
> +---
> +...
> +util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
> +---
> +...
> +_ = test_run:cmd("switch storage_1_a")
> +---
> +...
> +util = require('util')
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function wait_fibers_exit()
> + while #util.vshard_fiber_list() > 0 do
> + sleep(0.05)
4. Same.
> + end
> +end;
> +---> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 21093e5..da8e49e 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -33,6 +33,27 @@ if not M then
> }
> end
>
> +--
> +-- Destroy router module.
> +--
> +local function destroy()
> + local MODULE_INTERNALS = '__module_vshard_router'
> + assert(rawget(_G, MODULE_INTERNALS), 'Already destroyed')
> + local bg_fibers = {
> + 'failover_fiber',
> + 'discovery_fiber',
> + }
5. Please, just inline this cycle in 2 lines. Anyway on a new
fiber it is necessary to add a new name to bg_fibers.
> + for _, fib_name in pairs(bg_fibers) do
> + if M[fib_name] then
> + M[fib_name]:cancel()
> + M[fib_name] = nil
> + end
> + end
> + vshard.router.internal = nil
> + rawset(_G, MODULE_INTERNALS, nil)
> + M = nil
6. I do not like, that recfg now looks like
destroy
package.loaded = nil
cfg
It should not be necessary to nullify the package. Today in
the public chat a customer asked about
box.cfg{}
box.stop()
box.cfg{}
box.stop()
So there is no place for package.loaded. Most of people
even do not know about this thing existence.
> +end
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 059e705..ac37163 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -207,6 +207,43 @@ local function on_master_enable(...)
> end
> end
>
> +--------------------------------------------------------------------------------
> +-- Destroy
> +--------------------------------------------------------------------------------
> +
> +--
> +-- Destroy storage module.
> +--
> +local function destroy()
> + local MODULE_INTERNALS = '__module_vshard_storage'
> + assert(rawget(_G, MODULE_INTERNALS), 'Already destroyed')
> + box.space._bucket:drop()
7. When this instance is master for another instance (it will be so
when we return to master-master topology), this drop will be replicated
to active instances. Please, forbid destroy when the instance has active
relays.
> + for _, name in ipairs(storage_api) do
> + box.schema.func.drop(name)
> + end
> + local bg_fibers = {
> + 'recovery_fiber',
> + 'collect_bucket_garbage_fiber',
> + 'rebalancer_applier_fiber',
> + 'rebalancer_fiber',
> + }
> + for _, fib_name in pairs(bg_fibers) do
> + if M[fib_name] then
> + M[fib_name]:cancel()
> + M[fib_name] = nil
> + end
> + end
> + local box_cfg = table.deepcopy(box.cfg)
> + box_cfg.replication = nil
> + box_cfg.read_only = nil
8. Assign nil to table is like removal of the key. So here actually
you just recall empty box.cfg{}. To turn off the replication you
should pass empty replication array explicitly. To turn off the
read_only you should pass read_only = false explicitly.
> + box.cfg(box_cfg)
> + vshard.storage.internal = nil
> + box.space._schema:delete{'oncevshard:storage:1'}
> + rawset(_G, MODULE_INTERNALS, nil)
> + M = nil
> + box.snapshot()
9. Why?
> +end
> +
More information about the Tarantool-patches
mailing list