From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 30 Oct 2018 11:59:06 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [tarantool-patches] Re: [PATCH] box: fixed comparison of old and new config options Message-ID: <20181030085906.6evjnek636qjqtsl@esperanza> References: <1540886009.433331217@f173.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1540886009.433331217@f173.i.mail.ru> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: 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.