Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Alex Khatskevich <avkhatskevich@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options
Date: Tue, 26 Jun 2018 14:11:48 +0300	[thread overview]
Message-ID: <9d1392dc-c923-4e49-b3b5-41ad7d099ccd@tarantool.org> (raw)
In-Reply-To: <f334db42-6113-023d-9ec7-a8731c4ba864@tarantool.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 <avkhatskevich@tarantool.org>
> 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
>

  reply	other threads:[~2018-06-26 11:11 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-15 14:02 [tarantool-patches] " AKhatskevich
2018-06-21 14:26 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-25 11:48   ` Alex Khatskevich
2018-06-26 11:11     ` Vladislav Shpilevoy [this message]
2018-06-26 15:24       ` Alex Khatskevich
2018-06-27 12:03         ` Vladislav Shpilevoy
2018-06-27 12:59           ` Alex Khatskevich
2018-06-27 19:25           ` Alex Khatskevich
2018-06-28 11:06             ` Alex Khatskevich
2018-06-29 11:27               ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9d1392dc-c923-4e49-b3b5-41ad7d099ccd@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=avkhatskevich@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox