From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 75540469719 for ; Thu, 19 Mar 2020 01:32:19 +0300 (MSK) Date: Thu, 19 Mar 2020 01:26:02 +0300 From: Igor Munkin Message-ID: <20200318222602.GF6392@tarantool.org> References: <20191114115020.21091-1-maria.khaydich@tarantool.org> <1579344991.266039392@f319.i.mail.ru> <20200220175124.pwj24lx3jz4w6zcm@tkn_work_nb> <1583942274.319390956@f377.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1583942274.319390956@f377.i.mail.ru> Subject: Re: [Tarantool-patches] [PATCH 1/2] box: make box.cfg idempotent function List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maria Khaydich Cc: tarantool-patches@dev.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 . >   > 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); > 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   >   >   > -- > Maria Khaydich >   -- Best regards, IM