Re: [Tarantool-patches] [PATCH 1/2] box: make box.cfg idempotent function

[Igor Munkin <imun@tarantool.org>]

Masha,

Thanks for the patch! Please consider my comments below.

On 11.03.20, Maria Khaydich wrote:
> 
> Calling box.cfg{} more than once does not normally cause any errors
> (even though it might not have any effect). In contrast, assigning
> it to some variable and then using it after the box was configured
> caused an error since the method was overwritten by the initial call
> of <load_cfg>.
>  
> The patch fixes this issue making box.cfg behave consistently in both
> scenarios and is a follow-up for box: make box.execute idempotent function.
>  
> Follow-up #4231
> ---
> Issue:
> https://github.com/tarantool/tarantool/issues/4231  
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function  
>  
>  src/box/lua/load_cfg.lua                      | 12 ++++---
>  .../gh-4231-immutable-box-execute.test.lua    | 34 ++++++++++++-------
>  2 files changed, 30 insertions(+), 16 deletions(-)
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index dc4293bdd..6753be6f3 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -523,7 +523,15 @@ setmetatable(box, {
>       end
>  })
>  
> +local function box_is_configured()
> +    return ffi.C.box_is_configured()
> +end
> +

Minor: Since box_is_configured is introduced within this patchset it
could be placed properly in the first patch of the series. Feel free to
ignore.

>  local function load_cfg(cfg)
> +    if box_is_configured() then
> +        reload_cfg(cfg)
> +        return
> +    end
>      cfg = upgrade_cfg(cfg, translate_cfg)
>      cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
>      apply_default_cfg(cfg, default_cfg);

<snipped>

> diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua

Minor: test chunk name is left unchanged since the previous version and
doesn't respect the commit message wording.

> index 1544591bf..49aad154b 100755
> --- a/test/box-tap/gh-4231-immutable-box-execute.test.lua
> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
> @@ -1,28 +1,38 @@
>  #!/usr/bin/env tarantool
>  local tap = require('tap')
>  local test = tap.test('execute')
> -test:plan(2)
> +test:plan(4)
>  
>  --
> --- gh-4231: box.execute should be an idempotent function
> --- meaning its effect should be the same if a user chooses
> --- to use it before explicit box.cfg invocation
> +-- gh-4231: box.execute should be an idempotent function meaning
> +-- its effect should be the same if the user chooses to save it
> +-- before explicit box.cfg{} invocation and use the saved version
> +-- afterwards.
> +-- Within the scope of the same issue box.cfg method should also
> +-- be kept idempotent for the same reasons.
>  --
>  
> -local function execute_is_immutable(execute, cmd, msg)
> -    local status, err = pcall(execute, cmd)
> -    test:ok(status and type(err) == 'table', msg)
> -end
> -
>  local box_execute_stub = box.execute
> --- explicit call to load_cfg
> +local box_cfg_stub = box.cfg
> +
> +-- Explicit box configuration that used to change the behavior.
>  box.cfg{}
> +
>  local box_execute_actual = box.execute
> +local box_cfg_actual = box.cfg
>  
> -execute_is_immutable(box_execute_stub,
> +local function is_idempotent(method, cmd, msg)

Minor: IMHO these changes are better to be moved to the first patch of
the series. Feel free to ignore.

> +    local status, err = pcall(method, cmd)
> +    test:ok(status, msg, nil, err)
> +end
> +
> +is_idempotent(box_execute_stub,
>      "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",
>      "box.execute stub works before box.cfg")
> -execute_is_immutable(box_execute_actual, "DROP TABLE t1",
> +is_idempotent(box_execute_actual, "DROP TABLE t1",
>      "box.execute works properly after box.cfg")
>  
> +is_idempotent(box_cfg_stub, nil, "box.cfg stub works properly")
> +is_idempotent(box_cfg_actual, nil, "box.cfg after configuration")
> +
>  os.exit(test:check() and 0 or 1)
> -- 
> 2.24.0   

<snipped>

>  
>  
> --
> Maria Khaydich
>  

-- 
Best regards,
IM