Re: [Tarantool-patches] [PATCH vshard 2/5] storage: auto enable/disable

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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