Re: [tarantool-patches] Re: [tarantool-patches] Re: [PATCH] box: fixed comparison of old and new config options

[Vladimir Davydov <vdavydov.dev@gmail.com>]

Please don't tear the mailing thread apart and, even better, don't use
the web mailer.

On Tue, Oct 30, 2018 at 10:53:29AM +0300, Olga Arkhangelskaia wrote:
> 
> 
> 
> 29/10/2018 13:21, Vladimir Davydov пишет:
> > On Wed, Oct 24, 2018 at 01:14:31PM +0300, Olga Arkhangelskaia wrote:
> >> Due to incorrect configuration comparison, some options, especially
> >> replication were reseted and restarted, despite that change did not
> >> actually happened. The patch fixes it.
> >>
> >> 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
> >>
> >>   src/box/lua/load_cfg.lua       |  43 +++++++++++++++--
> >>   test/replication/misc.result   | 106 +++++++++++++++++++++++++++++++++++++++++
> >>   test/replication/misc.test.lua |  38 +++++++++++++++
> >>   3 files changed, 183 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> >> index df0c3c6ae..98d7ff47c 100644
> >> --- a/src/box/lua/load_cfg.lua
> >> +++ b/src/box/lua/load_cfg.lua
> >> @@ -365,6 +365,41 @@ local function apply_default_cfg(cfg, default_cfg)
> >>       end
> >>   end
> >> 
> >> +-- Compare given  config with old one.
> >> +-- Returns true if new config is similar to old one, false otherwise.
> > Nit: empty space between 'given' and 'config'. Articles are missing.
> > You should use 'equivalent' instead of 'similar' here.
> ok
> >
> >> +local function compare_cfg(cfg1, cfg2)
> >> +    if type(cfg1) ~= "table" and type(cfg2) ~= "table" then
> >> +	return cfg1 == cfg2
> >> +    end
> >> +    if type(cfg1) == "table" and type(cfg2) ~= "table" then
> >> +        if table.getn(cfg1) ~= 1 then
> > table.getn is deprecated, you should use # instead
> ok
> 
> >
> >> +            return false
> >> +        else
> >> +            if type(cfg1[1]) ~= "nil" and type(cfg2) ~= "nil" then
> >> +                return string.formagt(cfg1[1]) == string.format(cfg2)
> >> +            else return cfg1[1] == cfg2 end
> >> +        end
> >> +    end
> >> +    if type(cfg2) == "table" and type(cfg1) ~= "table" then
> >> +	if table.getn(cfg2) ~= 1 then
> >> +            return false
> >> +        else
> >> +            if type(cfg2[1]) ~= "nil" and type(cfg1) ~= "nil" then
> >> +                return string.format(cfg2[1]) == string.format(cfg1)
> >> +            else return cfg2[1] == cfg1 end
> >> +        end
> >> +    end
> > I don't think you should interpret the table content anyhow here, i.e.
> > convert values to string or treat a table of one element as a raw value,
> > because although this is valid for box.cfg.replication, this may be not
> > for other configuration options. This work should be done by modify_cfg.
> You mean additionally check in modify_cfg check type of values in 
> sub-tables of configuration?

I mean making modify_cfg.replication callback turn numbers to strings in
subtables (currently, it only does so with scalars).

> Yes, it is valid only for replication, but as I have seen - there is no 
> tables except replication config.

For now, yes. In future, we might want to add more. I want it to go
smoothly.

> They may have appear in future. But we still need compare value from 
> table and number sometimes.
> 
> >
> >> +    if type(cfg1) == "table" and type(cfg2) == "table" then
> >> +        if table.getn(cfg1) ~= table.getn(cfg2) then return false end
> >> +        table.sort(cfg1)
> >> +        table.sort(cfg2)
> >> +        for key in pairs(cfg1) do
> >> +	    if not compare_cfg(cfg1[key], cfg2[key]) then return false end
> >> +        end
> >> +	return true
> > Bad indentation.
> >
> > Anyway, this is a rather peculiar way of comparing tables. What if a
> > table contains subtables or string keys? How table.sort() is going to
> > work then? Please google a correct way of comparing Lua tables.
> At the moment I can't imagine situation when we have table for some 
> configuration option, that consists of
> subtables. Do you mean that some future changes may lead to such situations?
> I used table.sort when we need to compare {1,2} and {2, 1}. Should we 
> distinguish this configurations?

Yes, they are different. This issue is about skipping reload in case
replication configurations are identical, including the order.