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 D939925D8E for ; Thu, 7 Jun 2018 14:44:21 -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 MBiXnVPg38hE for ; Thu, 7 Jun 2018 14:44:21 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 9675025D39 for ; Thu, 7 Jun 2018 14:44:21 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] Add error messages References: From: Vladislav Shpilevoy Message-ID: <5795073d-1821-3c31-927e-4363829eeb67@tarantool.org> Date: Thu, 7 Jun 2018 21:44:19 +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 Thanks for the fixes! Finally the errors have become pretty. >> 4. Same as 2. > Fixed along with > "bucket moving to " -> "bucket moving to " What the difference? "bucket moving to" == "bucket moving to". >> >>>       end >>>       assert(not errcode) >>>       assert(mode == 'read' and bucket_is_readable(bucket) or >>>              mode == 'write' and bucket_is_writable(bucket)) >>> -::finish:: >>> +::wrong_bucket:: >> >> 6. The code below is executed both on error and on success. But >> label is named 'wrong_bucket'. It is not correct. On success >> path it is guaranteed that bucket ~= nil. > Renamed back to `finish`. IMHO `wrong_bucket` suited well here. You had labeled the success code as error processing. It is not ok. And in the same code you check 'if bucket and ...', but bucket on success is not nil - it is guaranteed. We must optimize the common case. And the bucket is ok most of time. +-- Only WRONG_BUCKET errors use this label. This is wrong. See three lines above - they process a case when the bucket is ok. You can label it ::wrong_bucket::, but then you must separate this code from success path. I refactored this function, you can look at it on the master branch. The patchset is pushed with my fixes.