[tarantool-patches] Re: [PATCH 3/3] Add error messages

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

Thanks for the patch! See 7 comments below.

On 06/06/2018 18:57, AKhatskevich wrote:
> Add error message to each `error_message_template`.
> This change required to change arguments of some errors to make
> them more informative.
> 
> Closes #100
> ---
>   test/router/reroute_wrong_bucket.result |  7 ++++---
>   test/router/router.result               | 16 +++++++++-------
>   test/storage/storage.result             |  3 ++-
>   vshard/error.lua                        | 15 +++++++++++----
>   vshard/router/init.lua                  |  3 +--
>   vshard/storage/init.lua                 | 28 ++++++++++++++++++++--------
>   6 files changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/test/router/reroute_wrong_bucket.result b/test/router/reroute_wrong_bucket.result
> index c7b980c..75a1c5a 100644
> --- a/test/router/reroute_wrong_bucket.result
> +++ b/test/router/reroute_wrong_bucket.result
> @@ -190,8 +190,9 @@ test_run:grep_log('router_1', 'please update configuration')
>   ...
>   err
>   ---
> -- {'bucket_id': 100, 'code': 1, 'type': 'ShardingError', 'destination': 'ac522f65-aa94-4134-9f64-51ee384f1a54',
> -  'message': 'WRONG_BUCKET: "{\"bucket_id\":100,\"destination\":\"ac522f65-aa94-4134-9f64-51ee384f1a54\"}"',
> +- {'bucket_id': 100, 'reason': 'bucket moving to ac522f65-aa94-4134-9f64-51ee384f1a54',
> +  'code': 1, 'type': 'ShardingError', 'destination': 'ac522f65-aa94-4134-9f64-51ee384f1a54',
> +  'message': 'Cannot perform action with bucket 100, reason: bucket moving to ac522f65-aa94-4134-9f64-51ee384f1a54',

1. WRONG_BUCKET should not expose 'reason' as an attribute.

> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 2d38ff9..cd02657 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -428,31 +428,43 @@ local function bucket_check_state(bucket_id, mode)
>       assert(mode == 'read' or mode == 'write')
>       local bucket = box.space._bucket:get({bucket_id})
>       local errcode = nil
> +    local reason = nil
>       if not bucket then
>           errcode = lerror.code.WRONG_BUCKET
> -        goto finish
> +        reason = 'Not found'
> +        goto wrong_bucket
>       elseif mode == 'read' then
>           if not bucket_is_readable(bucket) then
>               errcode = lerror.code.WRONG_BUCKET
> -            goto finish
> +            reason = 'read prohibited'

2. 'is prohibited'. No?

> +            goto wrong_bucket
>           end
>       elseif not bucket_is_writable(bucket) then
>           if bucket_is_transfer_in_progress(bucket) then
>               errcode = lerror.code.TRANSFER_IS_IN_PROGRESS
> +            return bucket, errcode and
> +                   lerror.vshard(errcode, bucket_id, bucket.destination)

3. How could 'errcode' be nil, if you had set it one line above?

>           else
>               errcode = lerror.code.WRONG_BUCKET
> +            reason = 'write prohibited'

4. Same as 2.

> +            goto wrong_bucket
>           end
> -        goto finish
>       elseif M.this_replicaset.master ~= M.this_replica then
>           errcode = lerror.code.NON_MASTER
> -        goto finish
> +        return bucket, errcode and
> +               lerror.vshard(errcode, M.this_replica.uuid,
> +                             M.this_replicaset.uuid)

5. Same as 3.

>       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.

7. Now msg is set for each error, but you still have checks in
vshard_error like 'if format.msg then'.