From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 449056ECC0; Sat, 18 Dec 2021 02:10:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 449056ECC0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1639782651; bh=zkcc+Xk6CFFGVRLO6CvVd0Jcb25DkLsjlc9/yKc1eHU=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=g+m8itjx8IarhFbYbs/8m6En8hRcDP2ZIB+K/F2w2qVHcvrjDcauHiL1Isg90Bqt2 EAjRwVoxyv6BHibCXp6cRVSILeqGQJh11zXO5BVJ6xSTj8JFuiEUZ38u/Bvi3b+xbx 2TThfLShXX2z7s1WsUNVvYXVU31s2HnmJN8Z1KtA= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 884016ECDC for ; Sat, 18 Dec 2021 02:10:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 884016ECDC Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1myMMq-0007Ro-Np; Sat, 18 Dec 2021 02:10:25 +0300 Message-ID: <405403ab-4eb9-56e9-1a3d-91035979c906@tarantool.org> Date: Sat, 18 Dec 2021 00:10:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Content-Language: en-US To: Oleg Babin , tarantool-patches@dev.tarantool.org References: <9d767a02cabc032ff4ad478b4a51c0a254276569.1639700518.git.v.shpilevoy@tarantool.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9B5397E24C93BDA67420B45F6C0BD1571DFA6B9070FC755DB182A05F5380850402C3F940FD559132FFE460E37780CFDDD6FCF4095EDC16BE898267E3CF4CA6AE4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CB1634DB9A2F7B99EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D07BBD2EBFB7BF888638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D86023FA992C3A66FB7C870B203F672AD8117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE4B6963042765DA4BBA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B6EF91E2CE43C37A3076E601842F6C81A1F004C906525384307823802FF610243DF43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C289736CE4F78F08343847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A55377B4097588051E0721A7636AE2B1139AB78CC1D26D43C2D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA759D2A03B9C34326B3410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D348BF433665406F3907B4736B19517C9D32BE43C9CDD9006D1EDE18B10A05AA21E70AFA9A12F2BF3A61D7E09C32AA3244CBACFED8D5F26B413415F1E05AFA88AD08894E9C85370243E729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojieEIankJUzptJQ8EVjhqfg== X-Mailru-Sender: 1F3202E75A95DDEFA8FDA09795F48B248655C2302DF3DFE5A7399CDCD88A1AD4E7FC8942C181312E07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH vshard 2/5] storage: auto enable/disable X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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