[tarantool-patches] Re: [PATCH 3/3] Introduce destroy module feature

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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
> +