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

[AKhatskevich <avkhatskevich@tarantool.org>]

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/test/misc/check_uuid_on_connect.result b/test/misc/check_uuid_on_connect.result
index d54c367..2695665 100644
--- a/test/misc/check_uuid_on_connect.result
+++ b/test/misc/check_uuid_on_connect.result
@@ -182,10 +182,12 @@ test_run:switch('bad_uuid_router')
 vshard.router.bucket_discovery(1)
 ---
 - null
-- type: ShardingError
-  unreachable_uuid: ac522f65-aa94-4134-9f64-51ee384f1a54
+- bucket_id: 1
   code: 8
-  bucket_id: 1
+  unreachable_uuid: ac522f65-aa94-4134-9f64-51ee384f1a54
+  message: There is no active replicas in replicaset ac522f65-aa94-4134-9f64-51ee384f1a54
+  name: UNREACHABLE_REPLICASET
+  type: ShardingError
 ...
 -- Ok to work with correct replicasets.
 vshard.router.bucket_discovery(2).uuid
diff --git a/test/router/reroute_wrong_bucket.result b/test/router/reroute_wrong_bucket.result
index eda3bab..c7b980c 100644
--- a/test/router/reroute_wrong_bucket.result
+++ b/test/router/reroute_wrong_bucket.result
@@ -109,9 +109,11 @@ test_run:switch('router_1')
 vshard.router.call(100, 'read', 'customer_lookup', {1}, {timeout = 1})
 ---
 - null
-- type: ShardingError
-  bucket_id: 100
+- bucket_id: 100
   code: 9
+  type: ShardingError
+  name: NO_ROUTE_TO_BUCKET
+  message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":100}"'
 ...
 -- Wait reconfiguration durigin timeout, if a replicaset was not
 -- found by bucket.destination from WRONG_BUCKET or
@@ -188,7 +190,9 @@ test_run:grep_log('router_1', 'please update configuration')
 ...
 err
 ---
-- {'type': 'ShardingError', 'bucket_id': 100, 'code': 1, 'destination': 'ac522f65-aa94-4134-9f64-51ee384f1a54'}
+- {'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\"}"',
+  'name': 'WRONG_BUCKET'}
 ...
 --
 -- Now try again, but update configuration during call(). It must
diff --git a/test/router/router.result b/test/router/router.result
index 5f3ceca..6955b59 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -195,15 +195,19 @@ vshard.router.bucket_count()
 util.check_error(vshard.router.call, 1, 'read', 'echo', {123})
 ---
 - null
-- type: ShardingError
-  bucket_id: 1
+- bucket_id: 1
   code: 9
+  type: ShardingError
+  name: NO_ROUTE_TO_BUCKET
+  message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":1}"'
 ...
 replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err
 ---
-- type: ShardingError
-  bucket_id: 1
+- bucket_id: 1
   code: 9
+  type: ShardingError
+  name: NO_ROUTE_TO_BUCKET
+  message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":1}"'
 ...
 vshard.router.bootstrap()
 ---
@@ -214,8 +218,9 @@ vshard.router.bootstrap()
 ---
 - null
 - type: ShardingError
-  code: 10
   message: Cluster is already bootstrapped
+  code: 10
+  name: NON_EMPTY
 ...
 --
 -- gh-108: negative bucket count on discovery.
@@ -312,9 +317,11 @@ util.check_error(vshard.router.call, 0, 'read', 'echo', {123})
 ...
 replicaset, err = vshard.router.bucket_discovery(0); return err == nil or err
 ---
-- type: ShardingError
-  bucket_id: 0
+- bucket_id: 0
   code: 9
+  type: ShardingError
+  name: NO_ROUTE_TO_BUCKET
+  message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":0}"'
 ...
 replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err
 ---
@@ -353,7 +360,9 @@ vshard.router.call(1, 'read', 'echo', {123})
 util.check_error(vshard.router.call, 1, 'write', 'echo', {123})
 ---
 - null
-- {'type': 'ShardingError', 'bucket_id': 1, 'code': 7, 'destination': '<replicaset_1>'}
+- {'bucket_id': 1, 'code': 7, 'type': 'ShardingError', 'destination': '<replicaset_1>',
+  'message': 'TRANSFER_IS_IN_PROGRESS: "{\"bucket_id\":1,\"destination\":\"<replicaset_1>\"}"',
+  'name': 'TRANSFER_IS_IN_PROGRESS'}
 ...
 test_run:cmd('switch storage_2_a')
 ---
@@ -397,10 +406,12 @@ test_run:cmd('stop server storage_2_a')
 util.check_error(vshard.router.call, 1, 'read', 'echo', {123})
 ---
 - null
-- type: ShardingError
-  unreachable_uuid: <replicaset_2>
+- bucket_id: 1
   code: 8
-  bucket_id: 1
+  unreachable_uuid: <replicaset_2>
+  message: There is no active replicas in replicaset <replicaset_2>
+  name: UNREACHABLE_REPLICASET
+  type: ShardingError
 ...
 vshard.router.buckets_info(0, 3)
 ---
@@ -420,9 +431,11 @@ test_run:cmd('start server storage_2_a')
 vshard.router.route(vshard.consts.DEFAULT_BUCKET_COUNT + 100)
 ---
 - null
-- type: ShardingError
-  bucket_id: 3100
+- bucket_id: 3100
   code: 9
+  type: ShardingError
+  name: NO_ROUTE_TO_BUCKET
+  message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":3100}"'
 ...
 util.check_error(vshard.router.route, 'asdfg')
 ---
@@ -956,7 +969,8 @@ test_run:cmd("switch router_1")
 util.check_error(vshard.router.call, 1, 'write', 'echo', { 'hello world' })
 ---
 - null
-- {'type': 'ShardingError', 'code': 2, 'bucket_id': 1}
+- {'bucket_id': 1, 'code': 2, 'type': 'ShardingError', 'name': 'NON_MASTER', 'message': 'NON_MASTER:
+    "{\"bucket_id\":1}"'}
 ...
 -- 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 e93ad24..aee9477 100644
--- a/test/storage/storage.result
+++ b/test/storage/storage.result
@@ -594,9 +594,11 @@ vshard.storage.call(1, 'read', 'customer_lookup', {1})
 vshard.storage.call(100500, 'read', 'customer_lookup', {1})
 ---
 - null
-- type: ShardingError
-  bucket_id: 100500
+- bucket_id: 100500
   code: 1
+  type: ShardingError
+  name: WRONG_BUCKET
+  message: 'WRONG_BUCKET: "{\"bucket_id\":100500}"'
 ...
 --
 -- Test not existing space in bucket data.
diff --git a/test/unit/error.result b/test/unit/error.result
index a46fb0d..ed9904a 100644
--- a/test/unit/error.result
+++ b/test/unit/error.result
@@ -20,13 +20,13 @@ tostring(box_error)
 ---
 - '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}'
 ...
-vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, {uuid = 'uuid', reason = 'reason'}, 'message 1 2 3')
+vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
 ---
 ...
 tostring(vshard_error)
 ---
-- '{"reason":"reason","code":11,"type":"ShardingError","uuid":"uuid","message":"message
-  1 2 3"}'
+- '{"reason":"reason","code":11,"type":"ShardingError","message":"Master of replicaset
+  uuid is unreachable: reason","uuid":"uuid","name":"UNREACHABLE_MASTER"}'
 ...
 log = require('log')
 ---
diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
index 4b5c20f..7b89f64 100644
--- a/test/unit/error.test.lua
+++ b/test/unit/error.test.lua
@@ -9,7 +9,7 @@ ok, err = pcall(box.error, box.error.TIMEOUT)
 box_error = lerror.box(err)
 tostring(box_error)
 
-vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, {uuid = 'uuid', reason = 'reason'}, 'message 1 2 3')
+vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
 tostring(vshard_error)
 
 log = require('log')
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.
+-- * msg -- Error message which can cantain parameters described
+--   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'}
+    },
+    [10] = {
+        name = 'NON_EMPTY',
+        msg = 'Cluster is already bootstrapped'
+    },
+    [11] = {
+        name = 'UNREACHABLE_MASTER',
+        msg = 'Master of replicaset %s is unreachable: %s',
+        args = {'uuid', 'reason'}
+    },
+    [12] = {
+        name = 'OUT_OF_SYNC',
+        msg = 'Replica is out of sync'
+    },
+    [13] = {
+        name = 'HIGH_REPLICATION_LAG',
+        msg = 'High replication lag: %f',
+        args = {'lag'}
+    },
+    [14] = {
+        name = 'UNREACHABLE_REPLICA',
+        msg = "Replica %s isn't active",
+        args = {'unreachable_uuid'}
+    },
+    [15] = {
+        name = 'LOW_REDUNDANCY',
+        msg = 'Only one replica is active'
+    },
+    [16] = {
+        name = 'INVALID_REBALANCING',
+        msg = 'Sending and receiving buckets at same time is not allowed'
+    },
+    [17] = {
+        name = 'SUBOPTIMAL_REPLICA',
+        msg = 'A current read replica in replicaset %s is not optimal'
+    },
+    [18] = {
+        name = 'UNKNOWN_BUCKETS',
+        msg = '%d buckets are not discovered',
+        arg = {'not_discovered_cnt'}
+    },
+    [19] = {
+        name = 'REPLICASET_IS_LOCKED',
+        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(error_code[err.name] == nil, "Dublicate error name")
+    error_code[err.name] = code
+end
+
 --
 -- There are 2 error types:
 -- * box_error - it is created on tarantool errors: client error,
@@ -11,19 +116,43 @@ local json = require('json')
 --   'ShardingError', one of codes below and optional
 --   message.
 --
-
 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.
+-- @param code Number, one of error_code constants.
+-- @param ... Arguments from `error_message_template` `arg` field.
+--        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
+    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)
+    else
+        msg = string.format('%s: %s', format.name, json.encode(tostring(ret)))
     end
+    ret.type = 'ShardingError'
+    ret.code = code
+    ret.name = format.name
+    ret.message = msg
     return ret
 end
 
@@ -45,70 +174,6 @@ local function make_error(e)
     end
 end
 
-local error_code = {
-    WRONG_BUCKET = 1,
-    NON_MASTER = 2,
-    BUCKET_ALREADY_EXISTS = 3,
-    NO_SUCH_REPLICASET = 4,
-    MOVE_TO_SELF = 5,
-    MISSING_MASTER = 6,
-    TRANSFER_IS_IN_PROGRESS = 7,
-    UNREACHABLE_REPLICASET = 8,
-    NO_ROUTE_TO_BUCKET = 9,
-    NON_EMPTY = 10,
-    UNREACHABLE_MASTER = 11,
-    OUT_OF_SYNC = 12,
-    HIGH_REPLICATION_LAG = 13,
-    UNREACHABLE_REPLICA = 14,
-    LOW_REDUNDANCY = 15,
-    INVALID_REBALANCING = 16,
-    SUBOPTIMAL_REPLICA = 17,
-    UNKNOWN_BUCKETS = 18,
-    REPLICASET_IS_LOCKED = 19,
-}
-
-local error_message_template = {
-    [error_code.MISSING_MASTER] = {
-         name = 'MISSING_MASTER',
-         msg = 'Master is not configured for replicaset %s'
-    },
-    [error_code.UNREACHABLE_MASTER] = {
-        name = 'UNREACHABLE_MASTER',
-        msg = 'Master of replicaset %s is unreachable: %s'
-    },
-    [error_code.OUT_OF_SYNC] = {
-        name = 'OUT_OF_SYNC',
-        msg = 'Replica is out of sync'
-    },
-    [error_code.HIGH_REPLICATION_LAG] = {
-        name = 'HIGH_REPLICATION_LAG',
-        msg = 'High replication lag: %f'
-    },
-    [error_code.UNREACHABLE_REPLICA] = {
-        name = 'UNREACHABLE_REPLICA',
-        msg = "Replica %s isn't active"
-    },
-    [error_code.UNREACHABLE_REPLICASET] = {
-        name = 'UNREACHABLE_REPLICASET',
-        msg = 'There is no active replicas in replicaset %s'
-    },
-    [error_code.LOW_REDUNDANCY] = {
-        name = 'LOW_REDUNDANCY',
-        msg = 'Only one replica is active'
-    },
-    [error_code.INVALID_REBALANCING] = {
-        name = 'INVALID_REBALANCING',
-        msg = 'Sending and receiving buckets at same time is not allowed'
-    },
-    [error_code.SUBOPTIMAL_REPLICA] = {
-        name = 'SUBOPTIMAL_REPLICA',
-        msg = 'A current read replica in replicaset %s is not optimal'
-    },
-    [error_code.UNKNOWN_BUCKETS] = {
-        name = 'UNKNOWN_BUCKETS',
-        msg = '%d buckets are not discovered',
-    }
-}
 
 local function make_alert(code, ...)
     local format = error_message_template[code]
diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
index 7fcf2df..99f59aa 100644
--- a/vshard/replicaset.lua
+++ b/vshard/replicaset.lua
@@ -114,8 +114,8 @@ end
 local function replicaset_connect_master(replicaset)
     local master = replicaset.master
     if master == nil then
-        return nil, lerror.vshard(lerror.code.MASTER_IS_MISSING,
-                                  {replicaset_uuid = replicaset.uuid})
+        return nil, lerror.vshard(lerror.code.MISSING_MASTER,
+                                  replicaset.uuid)
     end
     return replicaset_connect_to_replica(replicaset, master)
 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
@@ -85,8 +85,7 @@ local function bucket_discovery(bucket_id)
         if last_err.type == 'ClientError' and
            last_err.code == box.error.NO_CONNECTION then
             err = lerror.vshard(lerror.code.UNREACHABLE_REPLICASET,
-                                {bucket_id = bucket_id,
-                                 unreachable_uuid = unreachable_uuid})
+                                unreachable_uuid, bucket_id)
         else
             err = lerror.make(last_err)
         end
@@ -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)
     end
 
     return nil, err
@@ -519,8 +517,7 @@ local function cluster_bootstrap()
             return nil, err
         end
         if count > 0 then
-            return nil, lerror.vshard(lerror.code.NON_EMPTY, {},
-                                      'Cluster is already bootstrapped')
+            return nil, lerror.vshard(lerror.code.NON_EMPTY)
         end
     end
     lreplicaset.calculate_etalon_balance(M.replicasets, M.total_bucket_count)
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index d5e7b57..2d38ff9 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -452,8 +452,7 @@ local function bucket_check_state(bucket_id, mode)
            mode == 'write' and bucket_is_writable(bucket))
 ::finish::
     return bucket, errcode and
-           lerror.vshard(errcode, {bucket_id = bucket_id,
-                                   destination = bucket and bucket.destination})
+           lerror.vshard(errcode, bucket_id, bucket and bucket.destination)
 end
 
 --
@@ -514,8 +513,7 @@ local function bucket_recv(bucket_id, from, data)
 
     local bucket = box.space._bucket:get({bucket_id})
     if bucket ~= nil then
-        return nil, lerror.vshard(lerror.code.BUCKET_ALREADY_EXISTS,
-                                  {bucket_id = bucket_id})
+        return nil, lerror.vshard(lerror.code.BUCKET_ALREADY_EXISTS, bucket_id)
     end
 
     box.begin()
@@ -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
 
     if destination == box.info.cluster.uuid then
-        return nil, lerror.vshard(lerror.code.MOVE_TO_SELF,
-                                  {bucket_id = bucket_id,
-                                   replicaset_uuid = replicaset_uuid})
+        return nil, lerror.vshard(lerror.code.MOVE_TO_SELF, replicaset_uuid,
+                                  bucket_id)
     end
 
     local data = bucket_collect_internal(bucket_id)
@@ -1260,8 +1256,7 @@ end
 --
 local function rebalancer_apply_routes(routes)
     if is_this_replicaset_locked() then
-        return false, lerror.vshard(lerror.code.REPLICASET_IS_LOCKED, nil,
-                                    "Replicaset is locked");
+        return false, lerror.vshard(lerror.code.REPLICASET_IS_LOCKED);
     end
     assert(not rebalancing_is_in_progress())
     -- Can not apply routes here because of gh-946 in tarantool
-- 
2.14.1