* Re: [tarantool-patches] Re: [PATCH v2] box: fixed comparison of old and new config options
[not found] <1540974130.473038283@f273.i.mail.ru>
@ 2018-10-31 8:42 ` Vladimir Davydov
0 siblings, 0 replies; only message in thread
From: Vladimir Davydov @ 2018-10-31 8:42 UTC (permalink / raw)
To: Olga Arkhangelskaia; +Cc: tarantool-patches
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")
>
^ permalink raw reply [flat|nested] only message in thread