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 4D56525A70 for ; Thu, 7 Jun 2018 08:22:50 -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 7tGhy9904vKe for ; Thu, 7 Jun 2018 08:22:50 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 F167C25A5E for ; Thu, 7 Jun 2018 08:22:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/3] Refactor error reporting system References: From: Vladislav Shpilevoy Message-ID: <4ce919e8-117f-fee4-0adb-cd275de93d06@tarantool.org> Date: Thu, 7 Jun 2018 15:22:46 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, AKhatskevich Thanks for the patch! See 12 comments below. On 06/06/2018 18:57, AKhatskevich wrote: > This commit applies stricter rules on error reporting: > * error may has only arguments specified in `args` field of > `error_message_template` > * arguments may be used in error message > * arguments are saved in error object by names from `args` > > Part of #100 > --- > test/misc/check_uuid_on_connect.result | 8 +- > test/router/reroute_wrong_bucket.result | 10 +- > test/router/router.result | 42 ++++--- > test/storage/storage.result | 6 +- > test/unit/error.result | 6 +- > test/unit/error.test.lua | 2 +- > vshard/error.lua | 209 +++++++++++++++++++++----------- > vshard/replicaset.lua | 4 +- > vshard/router/init.lua | 11 +- > vshard/storage/init.lua | 17 +-- > 10 files changed, 197 insertions(+), 118 deletions(-) > > diff --git a/vshard/error.lua b/vshard/error.lua > index 8d26d11..1271d16 100644 > --- a/vshard/error.lua > +++ b/vshard/error.lua > @@ -1,6 +1,111 @@ > local ffi = require('ffi') > local json = require('json') > > +-- > +-- Error messages description. > +-- * name -- Key by which an error code can be retrieved from > +-- exprted `code` dictionary. 1. exprted -> ? > +-- * msg -- Error message which can cantain parameters described 2. cantain -> contain. And it must contain. Not can. > +-- in `string.format` notation. > +-- * args -- Names of arguments passed while constructing an > +-- error. After constructed, an error-object contains provided > +-- arguments by the names specified in the field. > +-- > +local error_message_template = { > + [1] = { > + name = 'WRONG_BUCKET', > + args = {'bucket_id', 'destination'} > + }, > + [2] = { > + name = 'NON_MASTER', > + args = {'bucket_id', 'destination'} > + }, > + [3] = { > + name = 'BUCKET_ALREADY_EXISTS', > + args = {'bucket_id'} > + }, > + [4] = { > + name = 'NO_SUCH_REPLICASET', > + args = {'replicaset_uuid'} > + }, > + [5] = { > + name = 'MOVE_TO_SELF', > + args = {'replicaset_uuid', 'bucket_id'} > + }, > + [6] = { > + name = 'MISSING_MASTER', > + msg = 'Master is not configured for replicaset %s', > + args = {'replicaset_uuid'} > + }, > + [7] = { > + name = 'TRANSFER_IS_IN_PROGRESS', > + args = {'bucket_id', 'destination'} > + }, > + [8] = { > + name = 'UNREACHABLE_REPLICASET', > + msg = 'There is no active replicas in replicaset %s', > + args = {'unreachable_uuid', 'bucket_id'} > + }, > + [9] = { > + name = 'NO_ROUTE_TO_BUCKET', > + args = {'bucket_id', 'unreachable_uuid'} 3. You have found that uuid is always nil for NO_ROUTE_TO_BUCKET. Why do you need it in args? > @@ -11,19 +116,43 @@ local json = require('json') > -- 'ShardingError', one of codes below and optional > -- message. > -- > - 4. Garbage diff. And this empty line was put here deliberately. The comment refers to both box_error and vshard_error. > local function box_error(original_error) > return setmetatable(original_error:unpack(), {__tostring = json.encode}) > end > > -local function vshard_error(code, args, msg) > - local ret = setmetatable({type = 'ShardingError', code = code, > - message = msg}, {__tostring = json.encode}) > - if args then > - for k, v in pairs(args) do > - ret[k] = v > - end > +-- > +-- Construct an vstard error. 5. vstard -> vshard. > +-- @param code Number, one of error_code constants. > +-- @param ... Arguments from `error_message_template` `arg` field. 6. arg -> args. Error objects above has args attribute. > +-- Caller have to pass at least as many arguments as > +-- `msg` field requires. > +-- @retval ShardingError object. > +-- > +local function vshard_error(code, ...) > + local format = error_message_template[code] > + assert(format, 'Error message format not found.') > + local args_passed_cnt = select('#', ...) > + local args = format.args or {} > + assert(#args >= args_passed_cnt, > + string.format('Wrong number of arguments passed to %s error', > + format.name))> + local ret = setmetatable({}, {__tostring = json.encode}) > + -- save error arguments 7. Please, start a sentence with capital letter and finish with dot. > + for i = 1, #args do > + ret[args[i]] = select(i, ...) > + end > + local msg > + if format.msg then > + local ok > + ok, msg = pcall(string.format, format.msg, ...) > + assert(ok) 8. Why? Either the pcall returns an error and you fail in the assert, or simple unprotected string.format() fails. No difference. Just to simple string.format(). 9. You do not need local msg here. > + else > + msg = string.format('%s: %s', format.name, json.encode(tostring(ret))) 10. Why do you need encode(tostring())? Ret already has __tostring = json.encode, so just do this: string.format('%s: %s', format.name, ret) Diff is below. I did not run the tests. Please, apply and fix the tests if needed. @@ -141,18 +141,14 @@ local function vshard_error(code, ...) for i = 1, #args do ret[args[i]] = select(i, ...) end - local msg if format.msg then - local ok - ok, msg = pcall(string.format, format.msg, ...) - assert(ok) + ret.message = string.format(format.msg, ...) else - msg = string.format('%s: %s', format.name, json.encode(tostring(ret))) + ret.message = string.format('%s: %s', format.name, ret) end ret.type = 'ShardingError' ret.code = code ret.name = format.name - ret.message = msg return ret end > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 42e9c86..95ee8b3 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -97,9 +96,8 @@ local function bucket_discovery(bucket_id) > -- bucket was found to be RECEIVING on one replicaset, and > -- was not found on other replicasets (it was sent during > -- discovery). > - err = lerror.vshard(lerror.code.NO_ROUTE_TO_BUCKET, > - {bucket_id = bucket_id, > - unreachable_uuid = unreachable_uuid}) > + err = lerror.vshard(lerror.code.NO_ROUTE_TO_BUCKET, bucket_id, > + unreachable_uuid) 11. Unreachable uuid is always nil as we have found. > end > > return nil, err > @@ -719,14 +717,12 @@ local function bucket_send(bucket_id, destination) > end > local replicaset = M.replicasets[destination] > if replicaset == nil then > - return nil, lerror.vshard(lerror.code.NO_SUCH_REPLICASET, > - {replicaset_uuid = destination}) > + return nil, lerror.vshard(lerror.code.NO_SUCH_REPLICASET, destination) > end 12. NO_SUCH_REPLICASET has two arguments. And you have assert(#args >= args_passed_cnt), that has not been failed. It means, that bucket_send has no a test on sending to non-existing replicaset. Please, * add a test on bucket_send to non-existing replicaset; * add unit tests on error building.