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 ED31C21E29 for ; Tue, 26 Jun 2018 07:11:50 -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 6dh4fVAbfEEr for ; Tue, 26 Jun 2018 07:11:50 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 A3B8221D75 for ; Tue, 26 Jun 2018 07:11:50 -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> From: Vladislav Shpilevoy Message-ID: <9d1392dc-c923-4e49-b3b5-41ad7d099ccd@tarantool.org> Date: Tue, 26 Jun 2018 14:11:48 +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 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. > > 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. > +... >  _ = 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. > +           error(string.format('Non-dynamic option %s ' .. > +                               'is changed in new vshard config', f_name)) > +        end > +    end >      return shard_cfg >  end >