<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_15409741120000000479_BODY">Thanks, wil fix.<br>
<br>
<br>
30/10/2018 19:13, Vladimir Davydov пишет:<br>
> On Tue, Oct 30, 2018 at 03:04:23PM +0300, Olga Arkhangelskaia wrote:<br>
>> Due to incorrect configuration comparison, some options, especially<br>
>> replication, were reseted and restarted, despite that configurations<br>
>> were equivalent.<br>
> Bad description. Please describe why exactly we're doing this, i.e. what<br>
> happens if we don't check whether box.cfg.replication actually changed.<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>
>> v1:<br>
>> <a href="https://www.freelists.org/post/tarantool-patches/PATCH-box-fixed-comparison-of-old-and-new-config-options" target="_blank">https://www.freelists.org/post/tarantool-patches/PATCH-box-fixed-comparison-of-old-and-new-config-options</a><br>
>><br>
>> Changes in v2:<br>
>> - changed test<br>
>> - conversion from num to string now in modify_cfg<br>
>> - no sort table in comparison<br>
>> - got rid of table.getn<br>
>><br>
>> src/box/lua/load_cfg.lua | 33 +++++++++++++++++++--<br>
>> test/replication/misc.result | 65 ++++++++++++++++++++++++++++++++++++++++++<br>
>> test/replication/misc.test.lua | 24 ++++++++++++++++<br>
>> 3 files changed, 119 insertions(+), 3 deletions(-)<br>
>><br>
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>
>> index df0c3c6ae..6d3c7f62c 100644<br>
>> --- a/src/box/lua/load_cfg.lua<br>
>> +++ b/src/box/lua/load_cfg.lua<br>
>> @@ -145,7 +145,11 @@ local template_cfg = {<br>
>> }<br>
>> <br>
>> local function normalize_uri(port)<br>
>> - if port == nil or type(port) == 'table' then<br>
>> + if port == nil then return port end<br>
> Nit: we never write 'then' and 'return' at the same line, please fix<br>
> here and everywhere.<br>
Good point, I used it a lot, sorry.<br>
<br>
><br>
>> + if type (port) == 'table' then<br>
> ^^^<br>
> Nit: extra space<br>
><br>
>> + for i in pairs(port) do<br>
>> + port[i] = tostring(port[i])<br>
>> + end<br>
>> return port<br>
>> end<br>
>> return tostring(port);<br>
> This function is also used for normalizing box.cfg.listen, which can't<br>
> be a table. IMHO we'd better introduce normalize_uri_list instead of<br>
> patching it.<br>
><br>
>> @@ -365,6 +369,29 @@ local function apply_default_cfg(cfg, default_cfg)<br>
>> end<br>
>> end<br>
>> <br>
>> +-- check whether two configurations are equivalent, returns true, if yes,<br>
>> +-- false otherwise.<br>
> Nit: we start comments with a capital letter and try to limit the text<br>
> width to ~65 symbols. Anyway, let's rewrite this comment to be as simple<br>
> as:<br>
><br>
> -- Return true if the two configurations are equivalent.<br>
><br>
>> +local function equivalent (t1, t2)<br>
> Please name this function compare_cfg() as you used to - this would be<br>
> consistent with load_cfg() and modify_cfg(). I wanted you to just fix<br>
> the comment.<br>
Ok<br>
<br>
><br>
>> + if type(t1) ~= 'table' and type(t2) ~= 'table' then return t1 == t2 end<br>
>> + if type(t1) == "table" and type(t2) ~= "table" then<br>
> Nit: please use the same quote style ("" or '') everywhere.<br>
><br>
>> + if #t1 == 1 then return t1[1] == t2 end<br>
> As I said, you should do all config interpretation in modify_cfg(),<br>
> in particular turning a scalar into a table if necessary.<br>
O misuderstood you here, I thought we do not need to change type of <br>
value in here.<br>
Now I see what should be done.<br>
<br>
><br>
>> + return false<br>
>> + end<br>
>> + if type(t2) == "table" and type(t1) ~= "table" then<br>
>> + if #t2 == 1 then return t2[1] == t1 end<br>
> ^^<br>
> Nit: extra space (it's very annoying, please self-review your code<br>
> before sending it for review).<br>
Again, sorry, try to do self review better.<br>
<br>
><br>
>> + return false<br>
>> + end<br>
>> + if type(t1) == 'table' and type(t2) == 'table' then<br>
>> + if #t1 ~= #t2 then return false end<br>
>> + for i in pairs(t2) do<br>
>> + if equivalent(t1[i], t2[i]) == false then<br>
> We don't use nested tables in configuration yet. Since this function<br>
> isn't generic anyway (it doesn't support Lua maps), I think we should<br>
> simplify it:<br>
Ok, band if we will use - we just change this?<br>
><br>
> for k, v in ipairs(t1) do<br>
> if t2[k] ~= v then<br>
> return false<br>
> end<br>
> end<br>
><br>
>> + return false<br>
>> + end<br>
>> + end<br>
>> + return true<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>
>> @@ -377,7 +404,7 @@ local function reload_cfg(oldcfg, cfg)<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 equivalent(val, oldval) then<br>
>> rawset(oldcfg, key, val)<br>
>> if not pcall(dynamic_cfg[key]) then<br>
>> rawset(oldcfg, key, oldval) -- revert the old value<br>
>> @@ -452,7 +479,7 @@ local function load_cfg(cfg)<br>
>> local val = cfg[key]<br>
>> if val ~= nil and not dynamic_cfg_skip_at_load[key] then<br>
>> fun()<br>
>> - if val ~= default_cfg[key] then<br>
>> + if not equivalent(val, default_cfg[key]) then<br>
>> log.info("set '%s' configuration option to %s", key, json.encode(val))<br>
>> end<br>
>> end<br>
>> diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua<br>
>> index 46726b7f4..7a3020f6e 100644<br>
>> --- a/test/replication/misc.test.lua<br>
>> +++ b/test/replication/misc.test.lua<br>
>> @@ -191,3 +191,27 @@ 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>
>> +--<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>
>> +replication = box.cfg.replication<br>
>> +test_run:cmd("switch default")<br>
>> +box.schema.user.revoke('guest', 'replication')<br>
> It would be nice if you described why you revoke replication privileges<br>
> here.<br>
><br>
>> +test_run:cmd("switch replica")<br>
>> +box.cfg{replication_sync_timeout = 0.5}<br>
> Why?<br>
><br>
>> +box.cfg{replication = {replication}}<br>
>> +box.info.status == 'running'<br>
>> +box.cfg{replication = replication}<br>
>> +box.info.status == 'running'<br>
> I want you to also check the case when box.cfg.replication is a table<br>
> of more than one element. You can use self as the second replication<br>
> source.<br>
><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>
<br>
</div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div></BODY></HTML>