* [tarantool-patches] [PATCH v2] box: fixed comparison of old and new config options
@ 2018-10-30 12:04 Olga Arkhangelskaia
2018-10-30 16:13 ` Vladimir Davydov
0 siblings, 1 reply; 2+ messages in thread
From: Olga Arkhangelskaia @ 2018-10-30 12:04 UTC (permalink / raw)
To: tarantool-patches; +Cc: Olga Arkhangelskaia
Due to incorrect configuration comparison, some options, especially
replication, were reseted and restarted, despite that configurations
were equivalent.
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
+ if type (port) == 'table' then
+ for i in pairs(port) do
+ port[i] = tostring(port[i])
+ end
return port
end
return tostring(port);
@@ -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.
+local function equivalent (t1, t2)
+ if type(t1) ~= 'table' and type(t2) ~= 'table' then return t1 == t2 end
+ if type(t1) == "table" and type(t2) ~= "table" then
+ if #t1 == 1 then return t1[1] == t2 end
+ return false
+ end
+ if type(t2) == "table" and type(t1) ~= "table" then
+ if #t2 == 1 then return t2[1] == t1 end
+ 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
+ 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.result b/test/replication/misc.result
index f8aa8dab6..2a1e9cffe 100644
--- a/test/replication/misc.result
+++ b/test/replication/misc.result
@@ -467,3 +467,68 @@ 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'")
+---
+- true
+...
+test_run:cmd("start server replica")
+---
+- true
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+replication = box.cfg.replication
+---
+...
+test_run:cmd("switch default")
+---
+- true
+...
+box.schema.user.revoke('guest', 'replication')
+---
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+box.cfg{replication_sync_timeout = 0.5}
+---
+...
+box.cfg{replication = {replication}}
+---
+...
+box.info.status == 'running'
+---
+- true
+...
+box.cfg{replication = replication}
+---
+...
+box.info.status == 'running'
+---
+- true
+...
+test_run:cmd("switch default")
+---
+- true
+...
+test_run:cmd("stop server replica")
+---
+- true
+...
+test_run:cmd("cleanup server replica")
+---
+- true
+...
+test_run:cmd("delete server replica")
+---
+- true
+...
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')
+test_run:cmd("switch replica")
+box.cfg{replication_sync_timeout = 0.5}
+box.cfg{replication = {replication}}
+box.info.status == 'running'
+box.cfg{replication = replication}
+box.info.status == 'running'
+
+test_run:cmd("switch default")
+test_run:cmd("stop server replica")
+test_run:cmd("cleanup server replica")
+test_run:cmd("delete server replica")
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [tarantool-patches] [PATCH v2] box: fixed comparison of old and new config options
2018-10-30 12:04 [tarantool-patches] [PATCH v2] box: fixed comparison of old and new config options Olga Arkhangelskaia
@ 2018-10-30 16:13 ` Vladimir Davydov
0 siblings, 0 replies; 2+ messages in thread
From: Vladimir Davydov @ 2018-10-30 16:13 UTC (permalink / raw)
To: Olga Arkhangelskaia; +Cc: tarantool-patches
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")
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-10-30 16:13 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 12:04 [tarantool-patches] [PATCH v2] box: fixed comparison of old and new config options Olga Arkhangelskaia
2018-10-30 16:13 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox