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 923822566C for ; Mon, 18 Jun 2018 06:48:11 -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 Nyv2AcMIxQYB for ; Mon, 18 Jun 2018 06:48:11 -0400 (EDT) Received: from smtp2.mail.ru (smtp2.mail.ru [94.100.179.91]) (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 4A4C724E60 for ; Mon, 18 Jun 2018 06:48:11 -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: Vladislav Shpilevoy Message-ID: Date: Mon, 18 Jun 2018 13:48:09 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Alex Khatskevich , tarantool-patches@freelists.org Hello. Thanks for the fixes! On 18/06/2018 13:10, Alex Khatskevich wrote: > >>> +-- 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. callro never raises (except OOM and usage error). So if here an error occurs, you will not see it. Same in other places below. > 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. I understand, but any error is impossible here after box.cfg{}. Please, move this injection one line up.