From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2DB3325B24 for ; Fri, 22 Jun 2018 18:11:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UsxOh5Vepzop for ; Fri, 22 Jun 2018 18:11:59 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B6AAB21C18 for ; Fri, 22 Jun 2018 18:11:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] Introduce destroy module feature References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 23 Jun 2018 01:11:55 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: AKhatskevich , tarantool-patches@freelists.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 > +