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 2AA0B216E8 for ; Wed, 27 Jun 2018 08:03:18 -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 AwtT7ikxYbaZ for ; Wed, 27 Jun 2018 08:03:18 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 BB4DF216D7 for ; Wed, 27 Jun 2018 08:03:17 -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: Vladislav Shpilevoy Message-ID: <161cd93f-0c72-181d-c9ab-23c9e8be1de3@tarantool.org> Date: Wed, 27 Jun 2018 15:03:15 +0300 MIME-Version: 1.0 In-Reply-To: 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: Alex Khatskevich , tarantool-patches@freelists.org Thanks for the fixes! The tests hangs on Travis (and locally too). On 26/06/2018 18:24, Alex Khatskevich wrote: > > > 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() >