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 17BB621E85 for ; Tue, 26 Jun 2018 11:24:35 -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 fezJNTIHRmRe for ; Tue, 26 Jun 2018 11:24:34 -0400 (EDT) Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 9CE8C21E2D for ; Tue, 26 Jun 2018 11:24:34 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options References: <20180615140225.2576-1-avkhatskevich@tarantool.org> <92d28edf-8237-c499-3ff0-e9795d8ff845@tarantool.org> <9d1392dc-c923-4e49-b3b5-41ad7d099ccd@tarantool.org> From: Alex Khatskevich Message-ID: Date: Tue, 26 Jun 2018 18:24:31 +0300 MIME-Version: 1.0 In-Reply-To: <9d1392dc-c923-4e49-b3b5-41ad7d099ccd@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Vladislav Shpilevoy , tarantool-patches@freelists.org On 26.06.2018 14:11, Vladislav Shpilevoy wrote: > Hello. Thanks for the fixes! See 4 comments below. > >>> 2. Same. And try to test here bucket_count instead of shard_index. >> fixed > > 1. Not fixed. I do not see a test for bucket_count. > Fixed >> >> commit 38d504e5d91835916618c743ba3b34ca0fd7ca22 >> Author: AKhatskevich >> Date:   Fri Jun 15 16:53:06 2018 +0300 >> >>      Prohibit reconfigure non-dynamic options >> >>      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 >> >> diff --git a/test/router/router.result b/test/router/router.result >> index 2ee1bff..5940e45 100644 >> --- a/test/router/router.result >> +++ b/test/router/router.result >> @@ -1057,6 +1057,17 @@ 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' >> +--- >> +... >> +util.check_error(vshard.router.cfg, non_dynamic_cfg) >> +--- >> +- Non-dynamic option shard_index is changed in new vshard config > > 2. This does not look like an error message. What is wrong here? Please, > rephrase to make it clear what is wrong. new msg ``` Non-dynamic option bucket_count cannot be reconfigured ``` > >> +... >>   _ = test_run:cmd("switch default") >>   --- >>   ...> diff --git a/vshard/cfg.lua b/vshard/cfg.lua >> index f5db4c0..f9d6d52 100644 >> --- a/vshard/cfg.lua >> +++ b/vshard/cfg.lua >> @@ -220,16 +220,34 @@ local cfg_template = { >>       }}, >>   } >> >> +-- >> +-- Names of options which cnnot be changed during reconfigure. > > 3. Typo. > >> +-- >> +local non_dynamic_options = { >> +    'bucket_count', 'shard_index' >> +} >> + >>   -- >>   -- 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 >> +    -- Check non-dynamic after default values are added. >> +    for _, f_name in pairs(non_dynamic_options) do >> +        -- New option may be added in new vshard version. >> +        if old_cfg[f_name] and shard_cfg[f_name] ~= old_cfg[f_name] >> then > > 4. See point 8 from the previous review. All the same. fixed Full diff: commit fdc7fd274844ea5e7bae1e0a212154b29c909504 Author: AKhatskevich Date:   Fri Jun 15 16:53:06 2018 +0300     Prohibit reconfigure non-dynamic options     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 diff --git a/test/router/router.result b/test/router/router.result index 2ee1bff..ab1808d 100644 --- a/test/router/router.result +++ b/test/router/router.result @@ -1057,6 +1057,17 @@ 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' +--- +... +util.check_error(vshard.router.cfg, non_dynamic_cfg) +--- +- Non-dynamic option shard_index cannot be reconfigured +...  _ = test_run:cmd("switch default")  ---  ... diff --git a/test/router/router.test.lua b/test/router/router.test.lua index fae8e24..63616cd 100644 --- a/test/router/router.test.lua +++ b/test/router/router.test.lua @@ -389,6 +389,11 @@ end;  test_run:cmd("setopt delimiter ''");  error_messages +-- gh-114: Check non-dynamic option change during reconfigure. +non_dynamic_cfg = table.copy(cfg) +non_dynamic_cfg.shard_index = 'non_default_name' +util.check_error(vshard.router.cfg, non_dynamic_cfg) +  _ = test_run:cmd("switch default")  test_run:drop_cluster(REPLICASET_2) diff --git a/test/storage/storage.result b/test/storage/storage.result index d0bf792..5685ccf 100644 --- a/test/storage/storage.result +++ b/test/storage/storage.result @@ -720,6 +720,17 @@ test_run:cmd("setopt delimiter ''");  ---  - true  ... +-- gh-114: Check non-dynamic option change during reconfigure. +non_dynamic_cfg = table.copy(cfg) +--- +... +non_dynamic_cfg.bucket_count = require('vshard.consts').DEFAULT_BUCKET_COUNT + 1 +--- +... +util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a) +--- +- Non-dynamic option bucket_count cannot be reconfigured +...  _ = test_run:cmd("switch default")  ---  ... diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua index f4bbf0e..72564e1 100644 --- a/test/storage/storage.test.lua +++ b/test/storage/storage.test.lua @@ -177,6 +177,11 @@ for _, new_replicaset in pairs(new_replicasets) do  end;  test_run:cmd("setopt delimiter ''"); +-- gh-114: Check non-dynamic option change during reconfigure. +non_dynamic_cfg = table.copy(cfg) +non_dynamic_cfg.bucket_count = require('vshard.consts').DEFAULT_BUCKET_COUNT + 1 +util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a) +  _ = test_run:cmd("switch default")  test_run:drop_cluster(REPLICASET_2) diff --git a/test/unit/config.result b/test/unit/config.result index 6b4f87b..904380b 100644 --- a/test/unit/config.result +++ b/test/unit/config.result @@ -506,3 +506,17 @@ _ = lcfg.check(cfg)  replica.name = 'storage'  ---  ... +-- gh-114: Check non-dynamic option change during reconfigure. +cfg_with_non_default = table.copy(cfg) +--- +... +cfg.shard_index = nil +--- +... +cfg_with_non_default.shard_index = 'non_default_name' +--- +... +util.check_error(lcfg.check, cfg, cfg_with_non_default) +--- +- Non-dynamic option shard_index cannot be reconfigured +... diff --git a/test/unit/config.test.lua b/test/unit/config.test.lua index 0f549d3..e0cd4f4 100644 --- a/test/unit/config.test.lua +++ b/test/unit/config.test.lua @@ -206,3 +206,9 @@ _ = lcfg.check(cfg)  replica.name = nil  _ = lcfg.check(cfg)  replica.name = 'storage' + +-- gh-114: Check non-dynamic option change during reconfigure. +cfg_with_non_default = table.copy(cfg) +cfg.shard_index = nil +cfg_with_non_default.shard_index = 'non_default_name' +util.check_error(lcfg.check, cfg, cfg_with_non_default) diff --git a/vshard/cfg.lua b/vshard/cfg.lua index f5db4c0..012060d 100644 --- a/vshard/cfg.lua +++ b/vshard/cfg.lua @@ -220,16 +220,34 @@ local cfg_template = {      }},  } +-- +-- Names of options which cannot be changed during reconfigure. +-- +local non_dynamic_options = { +    'bucket_count', 'shard_index' +} +  --  -- 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 +    -- Check non-dynamic after default values are added. +    for _, f_name in pairs(non_dynamic_options) do +        -- New option may be added in new vshard version. +        if shard_cfg[f_name] ~= old_cfg[f_name] then +           error(string.format('Non-dynamic option %s ' .. +                               'cannot be reconfigured', f_name)) +        end +    end      return shard_cfg  end diff --git a/vshard/router/init.lua b/vshard/router/init.lua index 21093e5..fc4714d 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.copy(cfg)      if not M.replicasets then          log.info('Starting router configuration')      else @@ -478,6 +479,7 @@ local function router_cfg(cfg)      -- TODO: update existing route map in-place      M.route_map = {}      M.replicasets = new_replicasets +    M.current_cfg = new_cfg      -- Move connections from an old configuration to a new one.      -- It must be done with no yields to prevent usage both of not      -- fully moved old replicasets, and not fully built new ones. diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua index 57076e1..8507a8a 100644 --- a/vshard/storage/init.lua +++ b/vshard/storage/init.lua @@ -1448,7 +1448,8 @@ local function storage_cfg(cfg, this_replica_uuid)      if this_replica_uuid == nil then          error('Usage: cfg(configuration, this_replica_uuid)')      end -    cfg = lcfg.check(cfg) +    cfg = lcfg.check(cfg, M.current_cfg) +    local new_cfg = table.copy(cfg)      if cfg.weights or cfg.zone then          error('Weights and zone are not allowed for storage configuration')      end @@ -1574,6 +1575,7 @@ local function storage_cfg(cfg, this_replica_uuid)      M.shard_index = shard_index      M.collect_bucket_garbage_interval = collect_bucket_garbage_interval      M.collect_lua_garbage = collect_lua_garbage +    M.current_cfg = new_cfg      if was_master and not is_master then          local_on_master_disable()