[tarantool-patches] Re: [PATCH 2/3] Refactor error reporting system

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.