From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D479E21C6A for ; Mon, 18 Jun 2018 06:10:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id aAcb0Cw2MzYC for ; Mon, 18 Jun 2018 06:10:58 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1EB1121381 for ; Mon, 18 Jun 2018 06:10:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/2] Add test on error during reconfigure References: <489ade011c878e28236afe2792e0eddb1ded75b9.1528566184.git.avkhatskevich@tarantool.org> <1fc9755e-0adc-8a44-4016-dedb7ba557b7@tarantool.org> From: Alex Khatskevich Message-ID: Date: Mon, 18 Jun 2018 13:10:54 +0300 MIME-Version: 1.0 In-Reply-To: <1fc9755e-0adc-8a44-4016-dedb7ba557b7@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , tarantool-patches@freelists.org >> +-- 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. Because next call checks a raised error. This line ensures that an error ocured only after `vshard.router.internal.errinj.ERRINJ_CFG = true` just by not failing. I did it to prevent test.result flooding. >> +--- >> +... >> +vshard.router.internal.errinj.ERRINJ_CFG = true >> +--- >> +... >> +old_internal = table.copy(vshard.router.internal) >> +--- >> +... >> +_, err = pcall(vshard.router.cfg, cfg) >> +--- > 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. The error I test here returns as a raised exception. > 3. Same. Here and at the end of the test. Same >> 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. In this case it does not matter, because it is just an errinj. However I wanted to show here, that error should be raised as late as possible. It can help in the future, if someone adds a code after `box.cfg`. I prefer errnj to be here and will move it only if you insist.