From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 1 Nov 2018 15:40:20 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v3] box: fixed comparison of old and new config options Message-ID: <20181101124020.l4c6zjkdfjyckc2x@esperanza> References: <20181031113214.79611-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181031113214.79611-1-arkholga@tarantool.org> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: On Wed, Oct 31, 2018 at 02:32:14PM +0300, Olga Arkhangelskaia wrote: > box.cfg() updates only those options that have actually changed. > However, for replication it is not always true: box.cfg{replication = x} > and box.cfg{replication = {x}} are treated differently, and as > the result - replication is restarted. The patch fixes such behaviour. > > 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 Please rebase on top of 1.10. > > v1: > https://www.freelists.org/post/tarantool-patches/PATCH-box-fixed-comparison-of-old-and-new-config-options > > v2: > https://www.freelists.org/post/tarantool-patches/PATCH-v2-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 > > Changes in v3: > - changed test > - fixed comments, names, identation > - added normalize_uri_list > > src/box/lua/load_cfg.lua | 39 +++++++++++++-- > test/replication/misc.result | 106 +++++++++++++++++++++++++++++++++++++++++ > test/replication/misc.test.lua | 41 ++++++++++++++++ > 3 files changed, 183 insertions(+), 3 deletions(-) > > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index df0c3c6ae..0dcb62f26 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -151,10 +151,25 @@ local function normalize_uri(port) > return tostring(port); > end > > +local function normalize_uri_list(port) The argument name is confusing. It's not a port, it's a list of URIs. > + if port == nil then > + return port > + end > + if type(port) == 'table' then > + if #port == 1 then > + return tostring(port[1]) > + end I'd expect normalize_uri_list() to always return a Lua table, i.e. it should turn 12345 into {'12345'} not vice versa. > + for i in pairs(port) do > + port[i] = tostring(port[i]) Modifying a table in-place isn't a good idea, because this might affect a user defined variable, e.g. replication = {1234, 5678} box.cfg{replication = replication} print(type(replication[1])) -- prints 'string' instead of 'number' Also, I think that instead of tostring() we should use normalize_uri() here, to emphasize that normalize_uri_list() normalizes each URI in a table. > + end > + return port > + end > + return tostring(port); > +end New line missing. > -- options that require special handling > local modify_cfg = { > listen = normalize_uri, > - replication = normalize_uri, > + replication = normalize_uri_list, > } > > local function purge_password_from_uri(uri) > @@ -365,6 +380,24 @@ local function apply_default_cfg(cfg, default_cfg) > end > end > > +-- Return true if two configurations are equivalent. > +local function compare_cfg(t1, t2) > + if type(t1) ~= 'table' and type(t2) ~= 'table' then > + return t1 == t2 > + end > + if type(t1) == 'table' and type(t2) == 'table' then > + if #t1 ~= #t2 then > + return false > + end > + for i, j in pairs(t2) do > + if t1[i] ~= j then > + return false > + end > + end > + return true > + end No return value in case t1 is a table and t2 is not or vice versa. > +end > + > local function reload_cfg(oldcfg, cfg) > cfg = upgrade_cfg(cfg, translate_cfg) > local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg) > diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua > index 46726b7f4..bf7408e96 100644 > --- a/test/replication/misc.test.lua > +++ b/test/replication/misc.test.lua > @@ -191,3 +191,44 @@ 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") > +-- Access rights are checked only during reconnect. If new config is equivalent > +-- to old one, replication will continue working. > +box.schema.user.revoke('guest', 'replication') > +test_run:cmd("switch replica") > +box.cfg{replication = {replication}} > +box.info.status == 'running' > +box.cfg{replication = replication} > +box.info.status == 'running' > + > +-- check table > +test_run:cmd("switch default") > +box.schema.user.grant('guest', 'replication') > +test_run:cmd("switch replica") > +listen = box.cfg.replication > +a = string.split(listen, ':') > +self = string.format('localhost:%s', a[2] + 1) > +box.cfg{listen = self, replication = {self, replication}} This doesn't work on Travis CI, check out https://travis-ci.org/tarantool/tarantool/builds/448792930?utm_source=github_status&utm_medium=notification You can simply use box.cfg.listen instead: replication = box.cfg.replication table.insert(replication, box.cfg.listen) > + > +test_run:cmd("switch default") > +box.schema.user.revoke('guest', 'replication') > + > +test_run:cmd("switch replica") > +box.cfg{replication = {self, replication}} > +box.info.status == 'running' > + > +test_run:cmd("switch default") > +test_run:cmd("stop server replica") > +test_run:cmd("cleanup server replica") > +test_run:cmd("delete server replica")