[tarantool-patches] Re: [PATCH 1/2] Add test on error during reconfigure
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Jun 17 22:46:46 MSK 2018
Thanks for the patch! See 4 comments below.
On 09/06/2018 20:47, AKhatskevich wrote:
> In case reconfigure process fails, the node should continue
> work properly.
> ---
> test/lua_libs/util.lua | 16 ++++++++++++++++
> test/router/router.result | 27 +++++++++++++++++++++++++++
> test/router/router.test.lua | 10 ++++++++++
> test/storage/storage.result | 33 +++++++++++++++++++++++++++++++++
> test/storage/storage.test.lua | 12 ++++++++++++
> vshard/router/init.lua | 7 +++++++
> vshard/storage/init.lua | 9 +++++++++
> 7 files changed, 114 insertions(+)
>
> diff --git a/test/router/router.result b/test/router/router.result
> index 2ee1bff..3ebab5d 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -1057,6 +1057,33 @@ error_messages
> - - Use replica:is_connected(...) instead of replica.is_connected(...)
> - Use replica:safe_uri(...) instead of replica.safe_uri(...)
> ...
> +-- Error during reconfigure process.
> +_ = vshard.router.route(1):callro('echo', {'some_data'})
1. Why do you need this call here? It does nor outputs
nothing.
> +---
> +...
> +vshard.router.internal.errinj.ERRINJ_CFG = true
> +---
> +...
> +old_internal = table.copy(vshard.router.internal)
> +---
> +...
> +_, err = pcall(vshard.router.cfg, cfg)
> +---
> +...
> +err:match('Error injection:.*')
> +---
> +- 'Error injection: cfg'
> +...
> +vshard.router.internal.errinj.ERRINJ_CFG = false
> +---
> +...
> +util.has_same_fields(old_internal, vshard.router.internal)
> +---
> +- true
> +...
> +_ = vshard.router.route(1):callro('echo', {'some_data'})
2. Same. Maybe this call should output something? This
test would not fail if callro failed. Call/ro/rw do not
throw exceptions but return a pair: result and error if
occurred.
> +---
> +...
> _ = test_run:cmd("switch default")
> ---
> ...
> diff --git a/test/storage/storage.result b/test/storage/storage.result
> index d0bf792..8d88bf4 100644
> --- a/test/storage/storage.result
> +++ b/test/storage/storage.result
> @@ -720,6 +720,39 @@ test_run:cmd("setopt delimiter ''");
> ---
> - true
> ...
> +-- Error during reconfigure process.
> +_, rs = next(vshard.storage.internal.replicasets)
> +---
> +...
> +_ = rs:callro('echo', {'some_data'})
3. Same. Here and at the end of the test.
> +---
> +...
> +vshard.storage.internal.errinj.ERRINJ_CFG = true
> +---
> +...
> +old_internal = table.copy(vshard.storage.internal)
> +---
> +...
> +_, err = pcall(vshard.storage.cfg, cfg, names.storage_1_a)
> +---
> +...
> +err:match('Error injection:.*')
> +---
> +- 'Error injection: cfg'
> +...
> +vshard.storage.internal.errinj.ERRINJ_CFG = false
> +---
> +...
> +util.has_same_fields(old_internal, vshard.storage.internal)
> +---
> +- true
> +...
> +_, rs = next(vshard.storage.internal.replicasets)
> +---
> +...
> +_ = rs:callro('echo', {'some_data'})
> +---
> +...
> _ = test_run:cmd("switch default")
> ---
> ...
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 21093e5..1dee80c 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -473,6 +474,12 @@ local function router_cfg(cfg)
> end
> box.cfg(cfg)
> log.info("Box has been configured")
> + -- It is considered that all possible errors during cfg
> + -- process occur only before this place.
> + -- This check should be placed as late as possible.
> + if M.errinj.ERRINJ_CFG then
> + error('Error injection: cfg')
> + end
4. I think, you should place this injection before box.cfg.
All the cfg() code relies on the fact, that box.cfg is atomic.
And if box.cfg is ok, then storage/router.cfg finish with
no errors. An error is possible only before box.cfg and in
box.cfg. You may think box.cfg like commit, and code before
box.cfg like prepare.
> M.total_bucket_count = total_bucket_count
> M.collect_lua_garbage = collect_lua_garbage
> -- TODO: update existing route map in-place
More information about the Tarantool-patches
mailing list