From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 31 Oct 2018 11:42:29 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v2] box: fixed comparison of old and new config options Message-ID: <20181031084229.gaiqbmtraanoiwr5@esperanza> References: <1540974130.473038283@f273.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1540974130.473038283@f273.i.mail.ru> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: Please keep the list in Cc and don't tear up the mailing thread. On Wed, Oct 31, 2018 at 11:22:10AM +0300, Olga Arkhangelskaia wrote: > > Thanks, wil fix. > > > 30/10/2018 19:13, Vladimir Davydov пишет: > > On Tue, Oct 30, 2018 at 03:04:23PM +0300, Olga Arkhangelskaia wrote: > >> Due to incorrect configuration comparison, some options, especially > >> replication, were reseted and restarted, despite that configurations > >> were equivalent. > > Bad description. Please describe why exactly we're doing this, i.e. what > > happens if we don't check whether box.cfg.replication actually changed. > > > >> Closes #3711 > >> --- > >> Issue: > >> https://github.com/tarantool/tarantool/issues/3711 > >> Branch: > >> https://github.com/tarantool/tarantool/tree/OKriw/gh-3711-do-not-restart-replication-if-config-did-not-change-2.1 > >> > >> v1: > >> https://www.freelists.org/post/tarantool-patches/PATCH-box-fixed-comparison-of-old-and-new-config-options > >> > >> Changes in v2: > >> - changed test > >> - conversion from num to string now in modify_cfg > >> - no sort table in comparison > >> - got rid of table.getn > >> > >> src/box/lua/load_cfg.lua | 33 +++++++++++++++++++-- > >> test/replication/misc.result | 65 ++++++++++++++++++++++++++++++++++++++++++ > >> test/replication/misc.test.lua | 24 ++++++++++++++++ > >> 3 files changed, 119 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > >> index df0c3c6ae..6d3c7f62c 100644 > >> --- a/src/box/lua/load_cfg.lua > >> +++ b/src/box/lua/load_cfg.lua > >> @@ -145,7 +145,11 @@ local template_cfg = { > >> } > >> > >> local function normalize_uri(port) > >> - if port == nil or type(port) == 'table' then > >> + if port == nil then return port end > > Nit: we never write 'then' and 'return' at the same line, please fix > > here and everywhere. > Good point, I used it a lot, sorry. > > > > >> + if type (port) == 'table' then > > ^^^ > > Nit: extra space > > > >> + for i in pairs(port) do > >> + port[i] = tostring(port[i]) > >> + end > >> return port > >> end > >> return tostring(port); > > This function is also used for normalizing box.cfg.listen, which can't > > be a table. IMHO we'd better introduce normalize_uri_list instead of > > patching it. > > > >> @@ -365,6 +369,29 @@ local function apply_default_cfg(cfg, default_cfg) > >> end > >> end > >> > >> +-- check whether two configurations are equivalent, returns true, if yes, > >> +-- false otherwise. > > Nit: we start comments with a capital letter and try to limit the text > > width to ~65 symbols. Anyway, let's rewrite this comment to be as simple > > as: > > > > -- Return true if the two configurations are equivalent. > > > >> +local function equivalent (t1, t2) > > Please name this function compare_cfg() as you used to - this would be > > consistent with load_cfg() and modify_cfg(). I wanted you to just fix > > the comment. > Ok > > > > >> + if type(t1) ~= 'table' and type(t2) ~= 'table' then return t1 == t2 end > >> + if type(t1) == "table" and type(t2) ~= "table" then > > Nit: please use the same quote style ("" or '') everywhere. > > > >> + if #t1 == 1 then return t1[1] == t2 end > > As I said, you should do all config interpretation in modify_cfg(), > > in particular turning a scalar into a table if necessary. > O misuderstood you here, I thought we do not need to change type of > value in here. > Now I see what should be done. > > > > >> + return false > >> + end > >> + if type(t2) == "table" and type(t1) ~= "table" then > >> + if #t2 == 1 then return t2[1] == t1 end > > ^^ > > Nit: extra space (it's very annoying, please self-review your code > > before sending it for review). > Again, sorry, try to do self review  better. > > > > >> + return false > >> + end > >> + if type(t1) == 'table' and type(t2) == 'table' then > >> + if #t1 ~= #t2 then return false end > >> + for i in pairs(t2) do > >> + if equivalent(t1[i], t2[i]) == false then > > We don't use nested tables in configuration yet. Since this function > > isn't generic anyway (it doesn't support Lua maps), I think we should > > simplify it: > Ok, band if we will use  - we just change this? Yes. > > > > for k, v in ipairs(t1) do > > if t2[k] ~= v then > > return false > > end > > end > > > >> + return false > >> + end > >> + end > >> + return true > >> + end > >> +end > >> + > >> local function reload_cfg(oldcfg, cfg) > >> cfg = upgrade_cfg(cfg, translate_cfg) > >> local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg) > >> @@ -377,7 +404,7 @@ local function reload_cfg(oldcfg, cfg) > >> for key in pairs(cfg) do > >> local val = newcfg[key] > >> local oldval = oldcfg[key] > >> - if oldval ~= val then > >> + if not equivalent(val, oldval) then > >> rawset(oldcfg, key, val) > >> if not pcall(dynamic_cfg[key]) then > >> rawset(oldcfg, key, oldval) -- revert the old value > >> @@ -452,7 +479,7 @@ local function load_cfg(cfg) > >> local val = cfg[key] > >> if val ~= nil and not dynamic_cfg_skip_at_load[key] then > >> fun() > >> - if val ~= default_cfg[key] then > >> + if not equivalent(val, default_cfg[key]) then > >> log.info("set '%s' configuration option to %s", key, json.encode(val)) > >> end > >> end > >> diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua > >> index 46726b7f4..7a3020f6e 100644 > >> --- a/test/replication/misc.test.lua > >> +++ b/test/replication/misc.test.lua > >> @@ -191,3 +191,27 @@ test_run:cmd("stop server replica") > >> test_run:cmd("cleanup server replica") > >> test_run:cmd("delete server replica") > >> box.schema.user.revoke('guest', 'replication') > >> + > >> + > >> +-- > >> +-- gh-3711 Do not restart replication on box.cfg if the configuration didn't change > >> +-- > >> +box.schema.user.grant('guest', 'replication') > >> + > >> +test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'") > >> +test_run:cmd("start server replica") > >> +test_run:cmd("switch replica") > >> +replication = box.cfg.replication > >> +test_run:cmd("switch default") > >> +box.schema.user.revoke('guest', 'replication') > > It would be nice if you described why you revoke replication privileges > > here. > > > >> +test_run:cmd("switch replica") > >> +box.cfg{replication_sync_timeout = 0.5} > > Why? > > > >> +box.cfg{replication = {replication}} > >> +box.info.status == 'running' > >> +box.cfg{replication = replication} > >> +box.info.status == 'running' > > I want you to also check the case when box.cfg.replication is a table > > of more than one element. You can use self as the second replication > > source. > > > >> + > >> +test_run:cmd("switch default") > >> +test_run:cmd("stop server replica") > >> +test_run:cmd("cleanup server replica") > >> +test_run:cmd("delete server replica") >