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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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