<HTML><BODY><div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<base target="_self" href="https://e.mail.ru/">
<div id="style_15408859880000000023_BODY"><br>
<br>
29/10/2018 13:21, Vladimir Davydov пишет:<br>
> On Wed, Oct 24, 2018 at 01:14:31PM +0300, Olga Arkhangelskaia wrote:<br>
>> Due to incorrect configuration comparison, some options, especially<br>
>> replication were reseted and restarted, despite that change did not<br>
>> actually happened. The patch fixes it.<br>
>><br>
>> Closes #3711<br>
>> ---<br>
>> Issue:<br>
>> <a href="https://github.com/tarantool/tarantool/issues/3711" target="_blank">https://github.com/tarantool/tarantool/issues/3711</a><br>
>> Branch:<br>
>> <a href="https://github.com/tarantool/tarantool/tree/OKriw/gh-3711-do-not-restart-replication-if-config-did-not-change-2.1" target="_blank">https://github.com/tarantool/tarantool/tree/OKriw/gh-3711-do-not-restart-replication-if-config-did-not-change-2.1</a><br>
>><br>
>> src/box/lua/load_cfg.lua | 43 +++++++++++++++--<br>
>> test/replication/misc.result | 106 +++++++++++++++++++++++++++++++++++++++++<br>
>> test/replication/misc.test.lua | 38 +++++++++++++++<br>
>> 3 files changed, 183 insertions(+), 4 deletions(-)<br>
>><br>
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>
>> index df0c3c6ae..98d7ff47c 100644<br>
>> --- a/src/box/lua/load_cfg.lua<br>
>> +++ b/src/box/lua/load_cfg.lua<br>
>> @@ -365,6 +365,41 @@ local function apply_default_cfg(cfg, default_cfg)<br>
>> end<br>
>> end<br>
>> <br>
>> +-- Compare given config with old one.<br>
>> +-- Returns true if new config is similar to old one, false otherwise.<br>
> Nit: empty space between 'given' and 'config'. Articles are missing.<br>
> You should use 'equivalent' instead of 'similar' here.<br>
ok<br>
><br>
>> +local function compare_cfg(cfg1, cfg2)<br>
>> + if type(cfg1) ~= "table" and type(cfg2) ~= "table" then<br>
>> + return cfg1 == cfg2<br>
>> + end<br>
>> + if type(cfg1) == "table" and type(cfg2) ~= "table" then<br>
>> + if table.getn(cfg1) ~= 1 then<br>
> table.getn is deprecated, you should use # instead<br>
ok<br>
<br>
><br>
>> + return false<br>
>> + else<br>
>> + if type(cfg1[1]) ~= "nil" and type(cfg2) ~= "nil" then<br>
>> + return string.formagt(cfg1[1]) == string.format(cfg2)<br>
>> + else return cfg1[1] == cfg2 end<br>
>> + end<br>
>> + end<br>
>> + if type(cfg2) == "table" and type(cfg1) ~= "table" then<br>
>> + if table.getn(cfg2) ~= 1 then<br>
>> + return false<br>
>> + else<br>
>> + if type(cfg2[1]) ~= "nil" and type(cfg1) ~= "nil" then<br>
>> + return string.format(cfg2[1]) == string.format(cfg1)<br>
>> + else return cfg2[1] == cfg1 end<br>
>> + end<br>
>> + end<br>
> I don't think you should interpret the table content anyhow here, i.e.<br>
> convert values to string or treat a table of one element as a raw value,<br>
> because although this is valid for box.cfg.replication, this may be not<br>
> for other configuration options. This work should be done by modify_cfg.<br>
You mean additionally check in modify_cfg check type of values in <br>
sub-tables of configuration?<br>
Yes, it is valid only for replication, but as I have seen - there is no <br>
tables except replication config.<br>
They may have appear in future. But we still need compare value from <br>
table and number sometimes.<br>
<br>
><br>
>> + if type(cfg1) == "table" and type(cfg2) == "table" then<br>
>> + if table.getn(cfg1) ~= table.getn(cfg2) then return false end<br>
>> + table.sort(cfg1)<br>
>> + table.sort(cfg2)<br>
>> + for key in pairs(cfg1) do<br>
>> + if not compare_cfg(cfg1[key], cfg2[key]) then return false end<br>
>> + end<br>
>> + return true<br>
> Bad indentation.<br>
><br>
> Anyway, this is a rather peculiar way of comparing tables. What if a<br>
> table contains subtables or string keys? How table.sort() is going to<br>
> work then? Please google a correct way of comparing Lua tables.<br>
At the moment I can't imagine situation when we have table for some <br>
configuration option, that consists of<br>
subtables. Do you mean that some future changes may lead to such situations?<br>
I used table.sort when we need to compare {1,2} and {2, 1}. Should we <br>
distinguish this configurations?<br>
<br>
><br>
>> + end<br>
>> +end<br>
>> +<br>
>> local function reload_cfg(oldcfg, cfg)<br>
>> cfg = upgrade_cfg(cfg, translate_cfg)<br>
>> local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)<br>
>> @@ -375,9 +410,9 @@ local function reload_cfg(oldcfg, cfg)<br>
>> end<br>
>> end<br>
>> for key in pairs(cfg) do<br>
>> - local val = newcfg[key]<br>
>> - local oldval = oldcfg[key]<br>
>> - if oldval ~= val then<br>
>> + if not compare_cfg(oldcfg[key], newcfg[key]) then<br>
>> + local val = newcfg[key]<br>
>> + local oldval = oldcfg[key]<br>
> Please try to keep your diff as small as possible. Here you could simply<br>
><br>
> local val = newcfg[key]<br>
> local oldval = oldcfg[key]<br>
> - if oldval ~= val then<br>
> + if not compare_cfg(oldval, val) then<br>
> rawset(oldcfg, key, val)<br>
><br>
> Also, you should use compare_cfg() in load_cfg() too.<br>
Sorry, missed it.<br>
<br>
><br>
>> rawset(oldcfg, key, val)<br>
>> if not pcall(dynamic_cfg[key]) then<br>
>> rawset(oldcfg, key, oldval) -- revert the old value<br>
>> @@ -388,7 +423,7 @@ local function reload_cfg(oldcfg, cfg)<br>
>> end<br>
>> log.info("set '%s' configuration option to %s", key,<br>
>> json.encode(val))<br>
>> - end<br>
>> + else log.info("option '%s' has not change", key) end<br>
> We silently skipped unmodified options before. I don't think we need to<br>
> log them now.<br>
Will remove, it mostly make test easier for me.<br>
<br>
><br>
>> end<br>
>> if type(box.on_reload_configuration) == 'function' then<br>
>> box.on_reload_configuration()<br>
>> diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua<br>
>> index 46726b7f4..96274b263 100644<br>
>> --- a/test/replication/misc.test.lua<br>
>> +++ b/test/replication/misc.test.lua<br>
>> @@ -191,3 +191,41 @@ test_run:cmd("stop server replica")<br>
>> test_run:cmd("cleanup server replica")<br>
>> test_run:cmd("delete server replica")<br>
>> box.schema.user.revoke('guest', 'replication')<br>
>> +<br>
>> +--<br>
>> +-- gh-3711 Do not restart replication on box.cfg if the configuration didn't change<br>
>> +--<br>
>> +box.schema.user.grant('guest', 'replication')<br>
>> +<br>
>> +test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'")<br>
>> +test_run:cmd("start server replica")<br>
>> +test_run:cmd("switch replica")<br>
>> +<br>
>> +test_run:grep_log('replica', 'replication')<br>
>> +replication = box.cfg.replication<br>
>> +box.cfg{replication = {replication}}<br>
>> +test_run:grep_log('replica', 'has not change') ~= nil<br>
>> +test_run:grep_log('replica', "set 'replication' configuration option to") == nil<br>
>> +box.cfg{replication = replication}<br>
>> +test_run:grep_log('replica', "set 'replication' configuration option to") == nil<br>
> There's a better way to check if this works: revoke replication grants<br>
> on the master and try to reconfigure replication on the replica.<br>
Will try, thanks.<br>
<br>
><br>
>> +<br>
>> +test_run:cmd("switch default")<br>
>> +test_run:cmd('stop server replica')<br>
>> +test_run:cmd('cleanup server test')<br>
>> +test_run:cmd("start server replica")<br>
>> +test_run:cmd("switch replica")<br>
>> +<br>
>> +listen = box.cfg.listen<br>
>> +replication = box.cfg.replication<br>
>> +<br>
>> +box.cfg{replication = {replication, listen}}<br>
>> +test_run:grep_log('replica', "set 'replication' configuration option to")<br>
>> +test_run:grep_log('replica', "has not change") == nil<br>
>> +box.cfg{replication = {listen, replication}}<br>
>> +test_run:grep_log('replica', "has not change") ~= nil<br>
>> +<br>
>> +test_run:cmd("switch default")<br>
>> +test_run:cmd("stop server replica")<br>
>> +test_run:cmd("cleanup server replica")<br>
>> +test_run:cmd("delete server replica")<br>
>> +box.schema.user.revoke('guest', 'replication')<br>
<br>
</div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div></BODY></HTML>