[Tarantool-patches] [PATCH vshard 2/5] storage: auto enable/disable
Oleg Babin
olegrok at tarantool.org
Sat Dec 18 16:58:03 MSK 2021
Thanks for your fixes. LGTM.
On 18.12.2021 02:10, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
> On 17.12.2021 12:09, Oleg Babin via Tarantool-patches wrote:
>> Thanks for your patch. See my two nits below.
>>
>> On 17.12.2021 03:25, Vladislav Shpilevoy wrote:
>>> +--------------------------------------------------------------------------------
>>> +-- Public API protection
>>> +--------------------------------------------------------------------------------
>>> +
>>> +--
>>> +-- Arguments are listed explicitly instead of '...' because the latter does not
>>> +-- jit.
>>> +--
>>> +local function storage_api_call_safe(func, arg1, arg2, arg3, arg4)
>>> + return func(arg1, arg2, arg3, arg4)
>>> +end
>>> +
>>> +--
>>> +-- Unsafe proxy is loaded with protections. But it is used rarely and only in
>>> +-- the beginning of instance's lifetime.
>>> +--
>>> +local function storage_api_call_unsafe(func, arg1, arg2, arg3, arg4)
>>> + -- box.info is quite expensive. Avoid calling it again when the instance
>>> + -- is finally loaded.
>>> + if not M.is_loaded then
>>> + -- box.info raises an error until box.cfg() is started.
>>> + local ok, status = pcall(function()
>>> + return box.info.status
>>> + end)
>> nit: It could be changed to type(box.cfg) == 'function'. I'd call it "common" pattern to check that box is not yet configured.
>>
>>> + if not ok then
>>> + local msg = 'box seem not to be configured'
>> nit: seem -> seems?
> Thanks, all fixed:
>
> ====================
> diff --git a/test/storage/storage.result b/test/storage/storage.result
> index e83b34f..790ba11 100644
> --- a/test/storage/storage.result
> +++ b/test/storage/storage.result
> @@ -967,15 +967,15 @@ _ = test_run:switch('storage_1_a')
> -- Leaving box.cfg() not called won't work because at 1.10 test-run somewhy
> -- raises an error when try to start an instance without box.cfg(). It can only
> -- be emulated.
> -old_info = box.info
> +old_cfg = box.cfg
> ---
> ...
> -box.info = setmetatable({}, {__index = function() error('not configured') end})
> +assert(type(old_cfg) == 'table')
> ---
> +- true
> ...
> -assert(not pcall(function() return box.info.status end))
> +box.cfg = function(...) return old_cfg(...) end
> ---
> -- true
> ...
> ok, err = pcall(vshard.storage.call, 1, 'read', 'echo', {100})
> ---
> @@ -984,11 +984,11 @@ assert(not ok and err.code == vshard.error.code.STORAGE_IS_DISABLED)
> ---
> - true
> ...
> -assert(err.message:match('box seem not to be configured') ~= nil)
> +assert(err.message:match('box seems not to be configured') ~= nil)
> ---
> - true
> ...
> -box.info = old_info
> +box.cfg = old_cfg
> ---
> ...
> -- Disabled until box is loaded.
> diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
> index ff39f2f..8695636 100644
> --- a/test/storage/storage.test.lua
> +++ b/test/storage/storage.test.lua
> @@ -309,14 +309,14 @@ _ = test_run:switch('storage_1_a')
> -- Leaving box.cfg() not called won't work because at 1.10 test-run somewhy
> -- raises an error when try to start an instance without box.cfg(). It can only
> -- be emulated.
> -old_info = box.info
> -box.info = setmetatable({}, {__index = function() error('not configured') end})
> -assert(not pcall(function() return box.info.status end))
> +old_cfg = box.cfg
> +assert(type(old_cfg) == 'table')
> +box.cfg = function(...) return old_cfg(...) end
>
> ok, err = pcall(vshard.storage.call, 1, 'read', 'echo', {100})
> assert(not ok and err.code == vshard.error.code.STORAGE_IS_DISABLED)
> -assert(err.message:match('box seem not to be configured') ~= nil)
> -box.info = old_info
> +assert(err.message:match('box seems not to be configured') ~= nil)
> +box.cfg = old_cfg
>
> -- Disabled until box is loaded.
> vshard.storage.internal.errinj.ERRINJ_CFG_DELAY = true
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index d3c4e2a..77da663 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -2953,14 +2953,11 @@ local function storage_api_call_unsafe(func, arg1, arg2, arg3, arg4)
> -- box.info is quite expensive. Avoid calling it again when the instance
> -- is finally loaded.
> if not M.is_loaded then
> - -- box.info raises an error until box.cfg() is started.
> - local ok, status = pcall(function()
> - return box.info.status
> - end)
> - if not ok then
> - local msg = 'box seem not to be configured'
> + if type(box.cfg) == 'function' then
> + local msg = 'box seems not to be configured'
> return error(lerror.vshard(lerror.code.STORAGE_IS_DISABLED, msg))
> end
> + local status = box.info.status
> -- 'Orphan' is allowed because even if a replica is an orphan, it still
> -- could be up to date. Just not all other replicas are connected.
> if status ~= 'running' and status ~= 'orphan' then
More information about the Tarantool-patches
mailing list