[tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options
Alex Khatskevich
avkhatskevich at tarantool.org
Tue Jun 26 18:24:31 MSK 2018
On 26.06.2018 14:11, Vladislav Shpilevoy wrote:
> Hello. Thanks for the fixes! See 4 comments below.
>
>>> 2. Same. And try to test here bucket_count instead of shard_index.
>> fixed
>
> 1. Not fixed. I do not see a test for bucket_count.
>
Fixed
>>
>> 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
>
> 2. This does not look like an error message. What is wrong here? Please,
> rephrase to make it clear what is wrong.
new msg
```
Non-dynamic option bucket_count cannot be reconfigured
```
>
>> +...
>> _ = test_run:cmd("switch 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.
>
> 3. Typo.
>
>> +--
>> +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
>
> 4. See point 8 from the previous review. All the same.
fixed
Full diff:
commit fdc7fd274844ea5e7bae1e0a212154b29c909504
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..ab1808d 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 cannot be reconfigured
+...
_ = 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..5685ccf 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.bucket_count =
require('vshard.consts').DEFAULT_BUCKET_COUNT + 1
+---
+...
+util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
+---
+- Non-dynamic option bucket_count cannot be reconfigured
+...
_ = test_run:cmd("switch default")
---
...
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
index f4bbf0e..72564e1 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.bucket_count =
require('vshard.consts').DEFAULT_BUCKET_COUNT + 1
+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..904380b 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 cannot be reconfigured
+...
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..012060d 100644
--- a/vshard/cfg.lua
+++ b/vshard/cfg.lua
@@ -220,16 +220,34 @@ local cfg_template = {
}},
}
+--
+-- Names of options which cannot 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 shard_cfg[f_name] ~= old_cfg[f_name] then
+ error(string.format('Non-dynamic option %s ' ..
+ 'cannot be reconfigured', 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