[tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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