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

• From: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
• To: Olga Arkhangelskaia <arkholga@xxxxxxxxxxxxx>
• Date: Tue, 30 Oct 2018 19:13:56 +0300

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.

+    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.

+    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.

+        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).

+        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:

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

• References:
  • [tarantool-patches] [PATCH v2] box: fixed comparison of old and new config options
    • From: Olga Arkhangelskaia

Other related posts:

• » [tarantool-patches] [PATCH v2] box: fixed comparison of old and new config options- Olga Arkhangelskaia
• » [tarantool-patches] Re: [PATCH v2] box: fixed comparison of old and new config options - Vladimir Davydov
• » [tarantool-patches] Re: [PATCH v2] box: fixed comparison of old and new config options- Vladimir Davydov

1. Home
2. tarantool-patches
3. Archive
4. 10-2018

---

• » Previous by Date
• » Next by Date
• » Related Posts
• » View list details
• » Manage your subscription

---