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

Olga Arkhangelskaia arkholga at tarantool.org
Wed Oct 31 11:22:10 MSK 2018


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?
>
>      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")

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20181031/2877ad3b/attachment.html>


More information about the Tarantool-patches mailing list