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

[Alex Khatskevich <avkhatskevich@tarantool.org>]

> 1. WRONG_BUCKET should not expose 'reason' as an attribute.
Discussed verbally. Let it just be exposed and useless and undocumented.
> 2. 'is prohibited'. No?
I just wanted short error messaged.
Fixed.
> 3. How could 'errcode' be nil, if you had set it one line above?
Thanks
> 4. Same as 2.
Fixed along with
"bucket moving to " -> "bucket moving to "
> 5. Same as 3.
Fixed
>
>>       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.
> 7. Now msg is set for each error, but you still have checks in
> vshard_error like 'if format.msg then'.
Fixed

New diff:

commit 057e640c25dfaab8ccafe3740cddaee27b2c7bc9
Author: AKhatskevich <avkhatskevich@tarantool.org>
Date:   Wed Jun 6 13:28:25 2018 +0300

     Add error messages

     Add error message to each `error_message_template`.
     This change required to change arguments of some errors to make
     them more informative.

     Closes #100

diff --git a/test/router/reroute_wrong_bucket.result 
b/test/router/reroute_wrong_bucket.result
index f8021f2..58bef8d 100644
--- a/test/router/reroute_wrong_bucket.result
+++ b/test/router/reroute_wrong_bucket.result
@@ -112,7 +112,7 @@ vshard.router.call(100, 'read', 'customer_lookup', 
{1}, {timeout = 1})
  - bucket_id: 100
    code: 9
    type: ShardingError
-  message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":100}'
+  message: Bucket 100 cannot be found. Is rebalancing in progress?
    name: NO_ROUTE_TO_BUCKET
  ...
  -- Wait reconfiguration durigin timeout, if a replicaset was not
@@ -190,8 +190,10 @@ test_run:grep_log('router_1', 'please update 
configuration')
  ...
  err
  ---
-- {'bucket_id': 100, 'code': 1, 'type': 'ShardingError', 'destination': 
'ac522f65-aa94-4134-9f64-51ee384f1a54',
-  'name': 'WRONG_BUCKET', 'message': 'WRONG_BUCKET: 
{"bucket_id":100,"destination":"ac522f65-aa94-4134-9f64-51ee384f1a54"}'}
+- {'bucket_id': 100, 'reason': 'bucket is moving to 
ac522f65-aa94-4134-9f64-51ee384f1a54',
+  'code': 1, 'destination': 'ac522f65-aa94-4134-9f64-51ee384f1a54', 
'type': 'ShardingError',
+  'name': 'WRONG_BUCKET', 'message': 'Cannot perform action with bucket 
100, reason:
+    bucket is moving to ac522f65-aa94-4134-9f64-51ee384f1a54'}
  ...
  --
  -- Now try again, but update configuration during call(). It must
diff --git a/test/router/router.result b/test/router/router.result
index 5d99255..2ee1bff 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -198,7 +198,7 @@ util.check_error(vshard.router.call, 1, 'read', 
'echo', {123})
  - bucket_id: 1
    code: 9
    type: ShardingError
-  message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":1}'
+  message: Bucket 1 cannot be found. Is rebalancing in progress?
    name: NO_ROUTE_TO_BUCKET
  ...
  replicaset, err = vshard.router.bucket_discovery(1); return err == nil 
or err
@@ -206,7 +206,7 @@ replicaset, err = vshard.router.bucket_discovery(1); 
return err == nil or err
  - bucket_id: 1
    code: 9
    type: ShardingError
-  message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":1}'
+  message: Bucket 1 cannot be found. Is rebalancing in progress?
    name: NO_ROUTE_TO_BUCKET
  ...
  vshard.router.bootstrap()
@@ -320,7 +320,7 @@ replicaset, err = vshard.router.bucket_discovery(0); 
return err == nil or err
  - bucket_id: 0
    code: 9
    type: ShardingError
-  message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":0}'
+  message: Bucket 0 cannot be found. Is rebalancing in progress?
    name: NO_ROUTE_TO_BUCKET
  ...
  replicaset, err = vshard.router.bucket_discovery(1); return err == nil 
or err
@@ -361,7 +361,8 @@ util.check_error(vshard.router.call, 1, 'write', 
'echo', {123})
  ---
  - null
  - {'bucket_id': 1, 'code': 7, 'type': 'ShardingError', 'destination': 
'<replicaset_1>',
-  'name': 'TRANSFER_IS_IN_PROGRESS', 'message': 
'TRANSFER_IS_IN_PROGRESS: {"bucket_id":1,"destination":"<replicaset_1>"}'}
+  'name': 'TRANSFER_IS_IN_PROGRESS', 'message': 'Bucket 1 is 
transferring to replicaset
+    <replicaset_1>'}
  ...
  test_run:cmd('switch storage_2_a')
  ---
@@ -433,7 +434,7 @@ 
vshard.router.route(vshard.consts.DEFAULT_BUCKET_COUNT + 100)
  - bucket_id: 3100
    code: 9
    type: ShardingError
-  message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":3100}'
+  message: Bucket 3100 cannot be found. Is rebalancing in progress?
    name: NO_ROUTE_TO_BUCKET
  ...
  util.check_error(vshard.router.route, 'asdfg')
@@ -968,8 +969,10 @@ test_run:cmd("switch router_1")
  util.check_error(vshard.router.call, 1, 'write', 'echo', { 'hello 
world' })
  ---
  - null
-- {'bucket_id': 1, 'code': 2, 'type': 'ShardingError', 'message': 
'NON_MASTER: {"bucket_id":1}',
-  'name': 'NON_MASTER'}
+- {'replica_uuid': '1e02ae8a-afc0-4e91-ba34-843a356b8ed7', 
'replicaset_uuid': '<replicaset_2>',
+  'type': 'ShardingError', 'message': 'Replica 
1e02ae8a-afc0-4e91-ba34-843a356b8ed7
+    is not a master for replicaset <replicaset_2> anymore',
+  'name': 'NON_MASTER', 'code': 2}
  ...
  -- Reconfigure router and test that the WRITE request does work
  vshard.router.cfg(cfg)
diff --git a/test/storage/storage.result b/test/storage/storage.result
index 51492a2..d0bf792 100644
--- a/test/storage/storage.result
+++ b/test/storage/storage.result
@@ -595,9 +595,10 @@ vshard.storage.call(100500, 'read', 
'customer_lookup', {1})
  ---
  - null
  - bucket_id: 100500
+  reason: Not found
    code: 1
    type: ShardingError
-  message: 'WRONG_BUCKET: {"bucket_id":100500}'
+  message: 'Cannot perform action with bucket 100500, reason: Not found'
    name: WRONG_BUCKET
  ...
  --
@@ -624,7 +625,7 @@ vshard.storage.bucket_send(1, 'unknown uuid')
    type: ShardingError
    name: NO_SUCH_REPLICASET
    replicaset_uuid: unknown uuid
-  message: 'NO_SUCH_REPLICASET: {"replicaset_uuid":"unknown uuid"}'
+  message: Replicaset unknown uuid not found
  ...
  -- Successful transfer.
  vshard.storage.bucket_send(1, replicaset2_uuid)
diff --git a/test/unit/error.result b/test/unit/error.result
index 74571ca..d1f04d0 100644
--- a/test/unit/error.result
+++ b/test/unit/error.result
@@ -41,14 +41,22 @@ test_run:grep_log('default', 
'"reason":"reason","code":11,"type":"ShardingError"
  --
  -- Part of gh-100: check `error.vshard`.
  --
-lerror.vshard(lerror.code.WRONG_BUCKET, 'arg1', 'arg2')
+lerror.vshard(lerror.code.WRONG_BUCKET, 1, 'arg2')
  ---
-- bucket_id: arg1
+- bucket_id: 1
+  reason: arg2
    code: 1
    type: ShardingError
+  message: 'Cannot perform action with bucket 1, reason: arg2'
    name: WRONG_BUCKET
-  message: 'WRONG_BUCKET: {"bucket_id":"arg1","destination":"arg2"}'
-  destination: arg2
+...
+-- Pass an arg of a wrong type.
+ok, res = pcall(lerror.vshard, lerror.code.WRONG_BUCKET, 'arg1', 'arg2')
+---
+...
+res:match('bad argument #2.*')
+---
+- 'bad argument #2 to ''format'' (number expected, got string)'
  ...
  -- Pass less args than msg requires.
  ok, res = pcall(lerror.vshard, lerror.code.MISSING_MASTER)
diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
index 7496982..ef9d5cd 100644
--- a/test/unit/error.test.lua
+++ b/test/unit/error.test.lua
@@ -19,7 +19,10 @@ test_run:grep_log('default', 
'"reason":"reason","code":11,"type":"ShardingError"
  --
  -- Part of gh-100: check `error.vshard`.
  --
-lerror.vshard(lerror.code.WRONG_BUCKET, 'arg1', 'arg2')
+lerror.vshard(lerror.code.WRONG_BUCKET, 1, 'arg2')
+-- Pass an arg of a wrong type.
+ok, res = pcall(lerror.vshard, lerror.code.WRONG_BUCKET, 'arg1', 'arg2')
+res:match('bad argument #2.*')
  -- Pass less args than msg requires.
  ok, res = pcall(lerror.vshard, lerror.code.MISSING_MASTER)
  res:match('bad argument #2.*')
diff --git a/vshard/error.lua b/vshard/error.lua
index 59e4fac..429e6da 100644
--- a/vshard/error.lua
+++ b/vshard/error.lua
@@ -14,23 +14,28 @@ local json = require('json')
  local error_message_template = {
      [1] = {
          name = 'WRONG_BUCKET',
-        args = {'bucket_id', 'destination'}
+        msg = 'Cannot perform action with bucket %d, reason: %s',
+        args = {'bucket_id', 'reason', 'destination'}
      },
      [2] = {
          name = 'NON_MASTER',
-        args = {'bucket_id', 'destination'}
+        msg = 'Replica %s is not a master for replicaset %s anymore',
+        args = {'replica_uuid', 'replicaset_uuid'}
      },
      [3] = {
          name = 'BUCKET_ALREADY_EXISTS',
+        msg = 'Bucket %d already exists',
          args = {'bucket_id'}
      },
      [4] = {
          name = 'NO_SUCH_REPLICASET',
+        msg = 'Replicaset %s not found',
          args = {'replicaset_uuid'}
      },
      [5] = {
          name = 'MOVE_TO_SELF',
-        args = {'replicaset_uuid', 'bucket_id'}
+        msg = 'Cannot move: bucket %d is already on replicaset %s',
+        args = {'bucket_id', 'replicaset_uuid'}
      },
      [6] = {
           name = 'MISSING_MASTER',
@@ -39,6 +44,7 @@ local error_message_template = {
      },
      [7] = {
          name = 'TRANSFER_IS_IN_PROGRESS',
+        msg = 'Bucket %d is transferring to replicaset %s',
          args = {'bucket_id', 'destination'}
      },
      [8] = {
@@ -48,6 +54,7 @@ local error_message_template = {
      },
      [9] = {
          name = 'NO_ROUTE_TO_BUCKET',
+        msg = 'Bucket %d cannot be found. Is rebalancing in progress?',
          args = {'bucket_id'}
      },
      [10] = {
@@ -95,13 +102,13 @@ local error_message_template = {
          msg = 'Replicaset is locked'
      }
  }
-
  --
  -- User-visible error_name -> error_number dictionary.
  --
  local error_code = {}
  for code, err in pairs(error_message_template) do
      assert(type(code) == 'number')
+    assert(err.msg, 'msg is required field')
      assert(error_code[err.name] == nil, "Dublicate error name")
      error_code[err.name] = code
  end
@@ -142,11 +149,7 @@ local function vshard_error(code, ...)
      for i = 1, #args do
          ret[args[i]] = select(i, ...)
      end
-    if format.msg then
-        ret.message = string.format(format.msg, ...)
-    else
-        ret.message = string.format('%s: %s', format.name, ret)
-    end
+    ret.message = string.format(format.msg, ...)
      ret.type = 'ShardingError'
      ret.code = code
      ret.name = format.name
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 2d38ff9..125b5b7 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -428,31 +428,42 @@ 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
+        reason = 'Not found'
          goto finish
      elseif mode == 'read' then
          if not bucket_is_readable(bucket) then
              errcode = lerror.code.WRONG_BUCKET
+            reason = 'read is prohibited'
              goto finish
          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, lerror.vshard(errcode, bucket_id, 
bucket.destination)
          else
              errcode = lerror.code.WRONG_BUCKET
+            reason = 'write is prohibited'
+            goto finish
          end
-        goto finish
      elseif M.this_replicaset.master ~= M.this_replica then
          errcode = lerror.code.NON_MASTER
-        goto finish
+        return bucket, lerror.vshard(errcode, M.this_replica.uuid,
+ M.this_replicaset.uuid)
      end
      assert(not errcode)
      assert(mode == 'read' and bucket_is_readable(bucket) or
             mode == 'write' and bucket_is_writable(bucket))
+-- Only WRONG_BUCKET errors use this label.
  ::finish::
+    if errcode and bucket and bucket.destination then
+        reason = "bucket is moving to " .. bucket.destination
+    end
      return bucket, errcode and
-           lerror.vshard(errcode, bucket_id, bucket and bucket.destination)
+           lerror.vshard(errcode, bucket_id, reason,
+                         bucket and bucket.destination)
  end

  --
@@ -721,8 +732,8 @@ local function bucket_send(bucket_id, destination)
      end

      if destination == box.info.cluster.uuid then
-        return nil, lerror.vshard(lerror.code.MOVE_TO_SELF, 
replicaset_uuid,
-                                  bucket_id)
+        return nil, lerror.vshard(lerror.code.MOVE_TO_SELF, bucket_id,
+                                  replicaset_uuid)
      end

      local data = bucket_collect_internal(bucket_id)