[tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options
Alex Khatskevich
avkhatskevich at tarantool.org
Mon Jun 25 14:48:53 MSK 2018
> On 15/06/2018 17:02, AKhatskevich wrote:
>> +err:match('Non%-dynamic.*')
>
> 1. Please, used util.check_error function instead of pcall + match.
>> +_, err = pcall(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
>
> 2. Same. And try to test here bucket_count instead of shard_index.
fixed
>> +non_dynamic_cfg_old = table.copy(cfg)
>
> 3. Why do you need here so many copies? Please, do not copy when
> possible. Tests are too long and big with no copies even.
ok
>> +err:match('Non%-dynamic.*')
>
> 4. Same as 1, 2: use util.check_error.
fixed
>
>>
>> -local function cfg_check(shard_cfg)
>> +local function cfg_check(shard_cfg, old_cfg)
>> if type(shard_cfg) ~= 'table' then
>> error('Сonfig must be map of options')
>> end
>> shard_cfg = table.deepcopy(shard_cfg)
>> validate_config(shard_cfg, cfg_template)
>> + if not old_cfg then
>> + return shard_cfg
>> + end
>> + assert(not old_cfg or type(old_cfg) == 'table')
>
> 5. Useless assert: 3 lines above we have already checked old_cfg to be
> not null. It is like
>
> if cond then
> -- ...
> else
> assert(not cond)
> end
deleted
>
> 6. This table will be created/destroyed on each cfg.check call. Please,
> move it out of the function like it is done for other option tables.
fixed
>
>> + for _, f_name in pairs(non_dynamic) do
>> + -- New option may be added in new vshard verion.
>
> 7. Typo.
fixed
>
> 8. Now VShard has no non-dynamic options but bucket_count and
> shard_index.
> And there are no ideas of new non-dynamic options. So please, just check
> old != new. We can add this check for new options when it will be needed.
>
>> + if old_cfg[f_name] and shard_cfg[f_name] ~= old_cfg[f_name]
>> then
ok
>> + cfg = lcfg.check(cfg, M.current_cfg)
>> + local new_cfg = table.deepcopy(cfg)
>
> 9. Why in tests do you copy, but deepcopy here?
Fixed, but I hope this patch will be applied after gh-116-reload, where
the same code is written.
I just was not sure if box.cfg changes the passed table and did deepcopy.
Full diff of the patch:
commit 38d504e5d91835916618c743ba3b34ca0fd7ca22
Author: AKhatskevich <avkhatskevich at tarantool.org>
Date: Fri Jun 15 16:53:06 2018 +0300
Prohibit reconfigure non-dynamic options
By now there are two non-dynamic options:
* bucket_count
* shard_index
They cannot be changed by passing a new value to
router/storage.cfg.
Closes #114
diff --git a/test/router/router.result b/test/router/router.result
index 2ee1bff..5940e45 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -1057,6 +1057,17 @@ error_messages
- - Use replica:is_connected(...) instead of replica.is_connected(...)
- Use replica:safe_uri(...) instead of replica.safe_uri(...)
...
+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+---
+...
+non_dynamic_cfg.shard_index = 'non_default_name'
+---
+...
+util.check_error(vshard.router.cfg, non_dynamic_cfg)
+---
+- Non-dynamic option shard_index is changed in new vshard config
+...
_ = test_run:cmd("switch default")
---
...
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index fae8e24..63616cd 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -389,6 +389,11 @@ end;
test_run:cmd("setopt delimiter ''");
error_messages
+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+non_dynamic_cfg.shard_index = 'non_default_name'
+util.check_error(vshard.router.cfg, non_dynamic_cfg)
+
_ = test_run:cmd("switch default")
test_run:drop_cluster(REPLICASET_2)
diff --git a/test/storage/storage.result b/test/storage/storage.result
index d0bf792..c6609f6 100644
--- a/test/storage/storage.result
+++ b/test/storage/storage.result
@@ -720,6 +720,17 @@ test_run:cmd("setopt delimiter ''");
---
- true
...
+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+---
+...
+non_dynamic_cfg.shard_index = 'non_default_name'
+---
+...
+util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
+---
+- Non-dynamic option shard_index is changed in new vshard config
+...
_ = test_run:cmd("switch default")
---
...
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
index f4bbf0e..1e1112b 100644
--- a/test/storage/storage.test.lua
+++ b/test/storage/storage.test.lua
@@ -177,6 +177,11 @@ for _, new_replicaset in pairs(new_replicasets) do
end;
test_run:cmd("setopt delimiter ''");
+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+non_dynamic_cfg.shard_index = 'non_default_name'
+util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
+
_ = test_run:cmd("switch default")
test_run:drop_cluster(REPLICASET_2)
diff --git a/test/unit/config.result b/test/unit/config.result
index 6b4f87b..d97f2b6 100644
--- a/test/unit/config.result
+++ b/test/unit/config.result
@@ -506,3 +506,17 @@ _ = lcfg.check(cfg)
replica.name = 'storage'
---
...
+-- gh-114: Check non-dynamic option change during reconfigure.
+cfg_with_non_default = table.copy(cfg)
+---
+...
+cfg.shard_index = nil
+---
+...
+cfg_with_non_default.shard_index = 'non_default_name'
+---
+...
+util.check_error(lcfg.check, cfg, cfg_with_non_default)
+---
+- Non-dynamic option shard_index is changed in new vshard config
+...
diff --git a/test/unit/config.test.lua b/test/unit/config.test.lua
index 0f549d3..e0cd4f4 100644
--- a/test/unit/config.test.lua
+++ b/test/unit/config.test.lua
@@ -206,3 +206,9 @@ _ = lcfg.check(cfg)
replica.name = nil
_ = lcfg.check(cfg)
replica.name = 'storage'
+
+-- gh-114: Check non-dynamic option change during reconfigure.
+cfg_with_non_default = table.copy(cfg)
+cfg.shard_index = nil
+cfg_with_non_default.shard_index = 'non_default_name'
+util.check_error(lcfg.check, cfg, cfg_with_non_default)
diff --git a/vshard/cfg.lua b/vshard/cfg.lua
index f5db4c0..f9d6d52 100644
--- a/vshard/cfg.lua
+++ b/vshard/cfg.lua
@@ -220,16 +220,34 @@ local cfg_template = {
}},
}
+--
+-- Names of options which cnnot be changed during reconfigure.
+--
+local non_dynamic_options = {
+ 'bucket_count', 'shard_index'
+}
+
--
-- Check sharding config on correctness. Check types, name and uri
--- uniqueness, master count (in each replicaset must by <= 1).
+-- uniqueness, master count (in each replicaset must be <= 1).
--
-local function cfg_check(shard_cfg)
+local function cfg_check(shard_cfg, old_cfg)
if type(shard_cfg) ~= 'table' then
error('Сonfig must be map of options')
end
shard_cfg = table.deepcopy(shard_cfg)
validate_config(shard_cfg, cfg_template)
+ if not old_cfg then
+ return shard_cfg
+ end
+ -- Check non-dynamic after default values are added.
+ for _, f_name in pairs(non_dynamic_options) do
+ -- New option may be added in new vshard version.
+ if old_cfg[f_name] and shard_cfg[f_name] ~= old_cfg[f_name] then
+ error(string.format('Non-dynamic option %s ' ..
+ 'is changed in new vshard config', f_name))
+ end
+ end
return shard_cfg
end
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 21093e5..fc4714d 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -457,7 +457,8 @@ end
--------------------------------------------------------------------------------
local function router_cfg(cfg)
- cfg = lcfg.check(cfg)
+ cfg = lcfg.check(cfg, M.current_cfg)
+ local new_cfg = table.copy(cfg)
if not M.replicasets then
log.info('Starting router configuration')
else
@@ -478,6 +479,7 @@ local function router_cfg(cfg)
-- TODO: update existing route map in-place
M.route_map = {}
M.replicasets = new_replicasets
+ M.current_cfg = new_cfg
-- Move connections from an old configuration to a new one.
-- It must be done with no yields to prevent usage both of not
-- fully moved old replicasets, and not fully built new ones.
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 57076e1..8507a8a 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -1448,7 +1448,8 @@ local function storage_cfg(cfg, this_replica_uuid)
if this_replica_uuid == nil then
error('Usage: cfg(configuration, this_replica_uuid)')
end
- cfg = lcfg.check(cfg)
+ cfg = lcfg.check(cfg, M.current_cfg)
+ local new_cfg = table.copy(cfg)
if cfg.weights or cfg.zone then
error('Weights and zone are not allowed for storage
configuration')
end
@@ -1574,6 +1575,7 @@ local function storage_cfg(cfg, this_replica_uuid)
M.shard_index = shard_index
M.collect_bucket_garbage_interval = collect_bucket_garbage_interval
M.collect_lua_garbage = collect_lua_garbage
+ M.current_cfg = new_cfg
if was_master and not is_master then
local_on_master_disable()
More information about the Tarantool-patches
mailing list