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 1272E25B8D for ; Thu, 21 Jun 2018 10:26:22 -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 j_ltViF-gts4 for ; Thu, 21 Jun 2018 10:26:22 -0400 (EDT) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 540A325B50 for ; Thu, 21 Jun 2018 10:26:21 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options References: <20180615140225.2576-1-avkhatskevich@tarantool.org> From: Vladislav Shpilevoy Message-ID: <92d28edf-8237-c499-3ff0-e9795d8ff845@tarantool.org> Date: Thu, 21 Jun 2018 17:26:19 +0300 MIME-Version: 1.0 In-Reply-To: <20180615140225.2576-1-avkhatskevich@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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 15/06/2018 17:02, AKhatskevich wrote: > By now there are two non-dynamic options: > * bucket_count > * shard_index > > They cannot be changed by passing a new value to > router/storage.cfg. > > Closes #114 > --- > Branch: https://github.com/tarantool/vshard/tree/kh/gh-114-non-dynamic > Issue: https://github.com/tarantool/vshard/issues/114 > > test/router/router.result | 14 ++++++++++++++ > test/router/router.test.lua | 6 ++++++ > test/storage/storage.result | 14 ++++++++++++++ > test/storage/storage.test.lua | 6 ++++++ > test/unit/config.result | 20 ++++++++++++++++++++ > test/unit/config.test.lua | 8 ++++++++ > vshard/cfg.lua | 19 +++++++++++++++++-- > vshard/router/init.lua | 4 +++- > vshard/storage/init.lua | 4 +++- > 9 files changed, 91 insertions(+), 4 deletions(-) > > diff --git a/test/router/router.result b/test/router/router.result > index 2ee1bff..1aacc0e 100644 > --- a/test/router/router.result > +++ b/test/router/router.result > @@ -1057,6 +1057,20 @@ error_messages > - - Use replica:is_connected(...) instead of replica.is_connected(...) > - Use replica:safe_uri(...) instead of replica.safe_uri(...) > ... > +-- gh-114: Check non-dynamic option change during reconfigure. > +non_dynamic_cfg = table.copy(cfg) > +--- > +... > +non_dynamic_cfg.shard_index = 'non_default_name' > +--- > +... > +_, err = pcall(vshard.router.cfg, non_dynamic_cfg) > +--- > +... > +err:match('Non%-dynamic.*') 1. Please, used util.check_error function instead of pcall + match. > +--- > +- Non-dynamic option shard_index is changed in new vshard config > +... > _ = test_run:cmd("switch default") > --- > ... > diff --git a/test/storage/storage.result b/test/storage/storage.result > index d0bf792..41e3af8 100644 > --- a/test/storage/storage.result > +++ b/test/storage/storage.result > @@ -720,6 +720,20 @@ test_run:cmd("setopt delimiter ''"); > --- > - true > ... > +-- gh-114: Check non-dynamic option change during reconfigure. > +non_dynamic_cfg = table.copy(cfg) > +--- > +... > +non_dynamic_cfg.shard_index = 'non_default_name' > +--- > +... > +_, err = pcall(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a) 2. Same. And try to test here bucket_count instead of shard_index. > +--- > +... > +err:match('Non%-dynamic.*') > +--- > +- Non-dynamic option shard_index is changed in new vshard config > +... > _ = test_run:cmd("switch default") > --- > ... > diff --git a/test/unit/config.result b/test/unit/config.result > index 6b4f87b..0d8c740 100644 > --- a/test/unit/config.result > +++ b/test/unit/config.result > @@ -506,3 +506,23 @@ _ = lcfg.check(cfg) > replica.name = 'storage' > --- > ... > +-- gh-114: Check non-dynamic option change during reconfigure. > +non_dynamic_cfg = table.copy(cfg) > +--- > +... > +non_dynamic_cfg_old = table.copy(cfg) 3. Why do you need here so many copies? Please, do not copy when possible. Tests are too long and big with no copies even. Here one copy is enough. > diff --git a/test/unit/config.test.lua b/test/unit/config.test.lua > index 0f549d3..972df65 100644 > --- a/test/unit/config.test.lua > +++ b/test/unit/config.test.lua > @@ -206,3 +206,11 @@ _ = lcfg.check(cfg) > replica.name = nil > _ = lcfg.check(cfg) > replica.name = 'storage' > + > +-- gh-114: Check non-dynamic option change during reconfigure. > +non_dynamic_cfg = table.copy(cfg) > +non_dynamic_cfg_old = table.copy(cfg) > +non_dynamic_cfg.shard_index = nil > +non_dynamic_cfg_old.shard_index = 'non_default_name' > +_, err = pcall(lcfg.check, non_dynamic_cfg, non_dynamic_cfg_old) > +err:match('Non%-dynamic.*') 4. Same as 1, 2: use util.check_error. > diff --git a/vshard/cfg.lua b/vshard/cfg.lua > index f5db4c0..8d26104 100644 > --- a/vshard/cfg.lua > +++ b/vshard/cfg.lua > @@ -222,14 +222,29 @@ local cfg_template = { > > -- > -- Check sharding config on correctness. Check types, name and uri > --- uniqueness, master count (in each replicaset must by <= 1). > +-- uniqueness, master count (in each replicaset must be <= 1). > -- > -local function cfg_check(shard_cfg) > +local function cfg_check(shard_cfg, old_cfg) > if type(shard_cfg) ~= 'table' then > error('Сonfig must be map of options') > end > shard_cfg = table.deepcopy(shard_cfg) > validate_config(shard_cfg, cfg_template) > + if not old_cfg then > + return shard_cfg > + end > + assert(not old_cfg or type(old_cfg) == 'table') 5. Useless assert: 3 lines above we have already checked old_cfg to be not null. It is like if cond then -- ... else assert(not cond) end > + -- Check non-dynamic after default values are added. > + local non_dynamic = { > + 'bucket_count', 'shard_index' > + } 6. This table will be created/destroyed on each cfg.check call. Please, move it out of the function like it is done for other option tables. > + for _, f_name in pairs(non_dynamic) do > + -- New option may be added in new vshard verion. 7. Typo. 8. Now VShard has no non-dynamic options but bucket_count and shard_index. And there are no ideas of new non-dynamic options. So please, just check old != new. We can add this check for new options when it will be needed. > + if old_cfg[f_name] and shard_cfg[f_name] ~= old_cfg[f_name] then > + error(string.format('Non-dynamic option %s ' .. > + 'is changed in new vshard config', f_name)) > + end > + end > return shard_cfg > end > > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 21093e5..f29998a 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -457,7 +457,8 @@ end > -------------------------------------------------------------------------------- > > local function router_cfg(cfg) > - cfg = lcfg.check(cfg) > + cfg = lcfg.check(cfg, M.current_cfg) > + local new_cfg = table.deepcopy(cfg) 9. Why in tests do you copy, but deepcopy here? > if not M.replicasets then > log.info('Starting router configuration') > else