* [tarantool-patches] [PATCH 0/3][vshard] Add error messages
@ 2018-06-06 15:57 AKhatskevich
2018-06-06 15:57 ` [tarantool-patches] [PATCH 1/3] Fix test on bucket_transferring error AKhatskevich
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: AKhatskevich @ 2018-06-06 15:57 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Branch: https://github.com/tarantool/vshard/tree/kh/gh-100-error-msg-wo-extra-data
Issue: https://github.com/tarantool/vshard/issues/100
*** BLURB HERE ***
AKhatskevich (3):
Fix test on bucket_transferring error
Refactor error reporting system
Add error messages
test/misc/check_uuid_on_connect.result | 8 +-
test/router/reroute_wrong_bucket.result | 11 +-
test/router/router.result | 54 +++++---
test/router/router.test.lua | 4 +-
test/storage/storage.result | 7 +-
test/unit/error.result | 6 +-
test/unit/error.test.lua | 2 +-
vshard/error.lua | 216 +++++++++++++++++++++-----------
vshard/replicaset.lua | 4 +-
vshard/router/init.lua | 10 +-
vshard/storage/init.lua | 39 +++---
11 files changed, 231 insertions(+), 130 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [tarantool-patches] [PATCH 1/3] Fix test on bucket_transferring error 2018-06-06 15:57 [tarantool-patches] [PATCH 0/3][vshard] Add error messages AKhatskevich @ 2018-06-06 15:57 ` AKhatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 2/3] Refactor error reporting system AKhatskevich ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: AKhatskevich @ 2018-06-06 15:57 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy --- test/router/router.result | 12 ++++++------ test/router/router.test.lua | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/router/router.result b/test/router/router.result index 101369a..5f3ceca 100644 --- a/test/router/router.result +++ b/test/router/router.result @@ -328,17 +328,17 @@ test_run:cmd('switch storage_2_a') --- - true ... -box.space._bucket:replace({1, vshard.consts.BUCKET.SENDING}) +box.space._bucket:replace({1, vshard.consts.BUCKET.SENDING, '<replicaset_1>'}) --- -- [1, 'sending'] +- [1, 'sending', '<replicaset_1>'] ... test_run:cmd('switch storage_1_a') --- - true ... -box.space._bucket:replace({1, vshard.consts.BUCKET.RECEIVING}) +box.space._bucket:replace({1, vshard.consts.BUCKET.RECEIVING, '<replicaset_2>'}) --- -- [1, 'receiving'] +- [1, 'receiving', '<replicaset_2>'] ... test_run:cmd('switch router_1') --- @@ -353,7 +353,7 @@ vshard.router.call(1, 'read', 'echo', {123}) util.check_error(vshard.router.call, 1, 'write', 'echo', {123}) --- - null -- {'type': 'ShardingError', 'code': 7, 'bucket_id': 1} +- {'type': 'ShardingError', 'bucket_id': 1, 'code': 7, 'destination': '<replicaset_1>'} ... test_run:cmd('switch storage_2_a') --- @@ -369,7 +369,7 @@ test_run:cmd('switch storage_1_a') ... box.space._bucket:delete({1}) --- -- [1, 'receiving'] +- [1, 'receiving', '<replicaset_2>'] ... test_run:cmd('switch router_1') --- diff --git a/test/router/router.test.lua b/test/router/router.test.lua index 62d6730..fae8e24 100644 --- a/test/router/router.test.lua +++ b/test/router/router.test.lua @@ -119,9 +119,9 @@ replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err replicaset, err = vshard.router.bucket_discovery(2); return err == nil or err test_run:cmd('switch storage_2_a') -box.space._bucket:replace({1, vshard.consts.BUCKET.SENDING}) +box.space._bucket:replace({1, vshard.consts.BUCKET.SENDING, 'cbf06940-0790-498b-948d-042b62cf3d29'}) test_run:cmd('switch storage_1_a') -box.space._bucket:replace({1, vshard.consts.BUCKET.RECEIVING}) +box.space._bucket:replace({1, vshard.consts.BUCKET.RECEIVING, 'ac522f65-aa94-4134-9f64-51ee384f1a54'}) test_run:cmd('switch router_1') -- Ok to read sending bucket. vshard.router.call(1, 'read', 'echo', {123}) -- 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH 2/3] Refactor error reporting system 2018-06-06 15:57 [tarantool-patches] [PATCH 0/3][vshard] Add error messages AKhatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 1/3] Fix test on bucket_transferring error AKhatskevich @ 2018-06-06 15:57 ` AKhatskevich 2018-06-07 12:22 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-06 15:57 ` [tarantool-patches] [PATCH 3/3] Add error messages AKhatskevich 2018-06-07 12:22 ` [tarantool-patches] Re: [PATCH 0/3][vshard] " Vladislav Shpilevoy 3 siblings, 1 reply; 11+ messages in thread From: AKhatskevich @ 2018-06-06 15:57 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/3] Refactor error reporting system 2018-06-06 15:57 ` [tarantool-patches] [PATCH 2/3] Refactor error reporting system AKhatskevich @ 2018-06-07 12:22 ` Vladislav Shpilevoy 2018-06-07 16:13 ` Alex Khatskevich 0 siblings, 1 reply; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-06-07 12:22 UTC (permalink / raw) To: tarantool-patches, 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/3] Refactor error reporting system 2018-06-07 12:22 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-06-07 16:13 ` Alex Khatskevich 0 siblings, 0 replies; 11+ messages in thread From: Alex Khatskevich @ 2018-06-07 16:13 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches >> +-- * name -- Key by which an error code can be retrieved from >> +-- exprted `code` dictionary. > 1. exprted -> ? Fixed. -- * name -- Key by which an error code can be retrieved from -- the expoted by the module `code` dictionary. >> +-- * msg -- Error message which can cantain parameters described > 2. cantain -> contain. And it must contain. Not can. fixed -- * msg -- Error message which can use `args` using -- `string.format` notation. > 3. You have found that uuid is always nil for NO_ROUTE_TO_BUCKET. > Why do you need it in args? Fixed > 4. Garbage diff. And this empty line was put here deliberately. > The comment refers to both box_error and vshard_error. fixed. > 5. vstard -> vshard. Fixed > 6. arg -> args. Error objects above has args attribute. fixed > 7. Please, start a sentence with capital letter and finish with dot. 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(). fixed. > 9. You do not need local msg here. fixed >> + 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 fixed > 11. Unreachable uuid is always nil as we have found. Fixed >> 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. New diff: commit af0bfccfbba87a8dcccd71f0dd7d2f8b6c2317cf Author: AKhatskevich <avkhatskevich@tarantool.org> Date: Tue Jun 5 17:05:15 2018 +0300 Refactor error reporting system 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` Extra: * testcase for NO_SUCH_REPLICASET added Part of #100 diff --git a/test/misc/check_uuid_on_connect.result b/test/misc/check_uuid_on_connect.result index d54c367..6ac2353 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 + name: UNREACHABLE_REPLICASET + message: There is no active replicas in replicaset ac522f65-aa94-4134-9f64-51ee384f1a54 + 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..f8021f2 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 + message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":100}' + name: NO_ROUTE_TO_BUCKET ... -- Wait reconfiguration durigin timeout, if a replicaset was not -- found by bucket.destination from WRONG_BUCKET or @@ -188,7 +190,8 @@ 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', + 'name': 'WRONG_BUCKET', 'message': 'WRONG_BUCKET: {"bucket_id":100,"destination":"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 5f3ceca..5d99255 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 + message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":1}' + name: NO_ROUTE_TO_BUCKET ... replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err --- -- type: ShardingError - bucket_id: 1 +- bucket_id: 1 code: 9 + type: ShardingError + message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":1}' + name: NO_ROUTE_TO_BUCKET ... vshard.router.bootstrap() --- @@ -214,8 +218,9 @@ vshard.router.bootstrap() --- - null - type: ShardingError - code: 10 + name: NON_EMPTY message: Cluster is already bootstrapped + code: 10 ... -- -- 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 + message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":0}' + name: NO_ROUTE_TO_BUCKET ... replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err --- @@ -353,7 +360,8 @@ 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>', + 'name': 'TRANSFER_IS_IN_PROGRESS', 'message': 'TRANSFER_IS_IN_PROGRESS: {"bucket_id":1,"destination":"<replicaset_1>"}'} ... test_run:cmd('switch storage_2_a') --- @@ -397,10 +405,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> + name: UNREACHABLE_REPLICASET + message: There is no active replicas in replicaset <replicaset_2> + type: ShardingError ... vshard.router.buckets_info(0, 3) --- @@ -420,9 +430,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 + message: 'NO_ROUTE_TO_BUCKET: {"bucket_id":3100}' + name: NO_ROUTE_TO_BUCKET ... util.check_error(vshard.router.route, 'asdfg') --- @@ -956,7 +968,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', 'message': 'NON_MASTER: {"bucket_id":1}', + 'name': 'NON_MASTER'} ... -- 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..51492a2 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 + message: 'WRONG_BUCKET: {"bucket_id":100500}' + name: WRONG_BUCKET ... -- -- Test not existing space in bucket data. @@ -614,6 +616,17 @@ vshard.storage.bucket_recv(100, 'from_uuid', {{1000, {{1}}}}) -- -- Bucket transfer -- +-- Transfer to unknown replicaset. +vshard.storage.bucket_send(1, 'unknown uuid') +--- +- null +- code: 4 + type: ShardingError + name: NO_SUCH_REPLICASET + replicaset_uuid: unknown uuid + message: 'NO_SUCH_REPLICASET: {"replicaset_uuid":"unknown uuid"}' +... +-- Successful transfer. vshard.storage.bucket_send(1, replicaset2_uuid) --- - true diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua index 948decc..f4bbf0e 100644 --- a/test/storage/storage.test.lua +++ b/test/storage/storage.test.lua @@ -137,6 +137,11 @@ vshard.storage.bucket_recv(100, 'from_uuid', {{1000, {{1}}}}) -- -- Bucket transfer -- + +-- Transfer to unknown replicaset. +vshard.storage.bucket_send(1, 'unknown uuid') + +-- Successful transfer. vshard.storage.bucket_send(1, replicaset2_uuid) _ = test_run:cmd("switch storage_2_a") vshard.storage.buckets_info() diff --git a/test/unit/error.result b/test/unit/error.result index a46fb0d..74571ca 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","name":"UNREACHABLE_MASTER","uuid":"uuid","message":"Master + of replicaset uuid is unreachable: reason"}' ... log = require('log') --- @@ -38,6 +38,42 @@ test_run:grep_log('default', '"reason":"reason","code":11,"type":"ShardingError" --- - '"reason":"reason","code":11,"type":"ShardingError"' ... +-- +-- Part of gh-100: check `error.vshard`. +-- +lerror.vshard(lerror.code.WRONG_BUCKET, 'arg1', 'arg2') +--- +- bucket_id: arg1 + code: 1 + type: ShardingError + name: WRONG_BUCKET + message: 'WRONG_BUCKET: {"bucket_id":"arg1","destination":"arg2"}' + destination: arg2 +... +-- Pass less args than msg requires. +ok, res = pcall(lerror.vshard, lerror.code.MISSING_MASTER) +--- +... +res:match('bad argument #2.*') +--- +- 'bad argument #2 to ''format'' (no value)' +... +-- Pass more args than `args` field contains. +ok, res = pcall(lerror.vshard, lerror.code.MISSING_MASTER, 'arg1', 'arg2') +--- +... +res:match('Wrong number of.*') +--- +- Wrong number of arguments passed to MISSING_MASTER error +... +-- Pass wrong format code. +ok, res = pcall(lerror.vshard, 'Wrong format code', 'arg1', 'arg2') +--- +... +res:match('Error message format not found.') +--- +- Error message format not found. +... function raise_lua_err() assert(false) end --- ... diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua index 4b5c20f..7496982 100644 --- a/test/unit/error.test.lua +++ b/test/unit/error.test.lua @@ -9,13 +9,27 @@ 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') log.info('Log error: %s', vshard_error) 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') +-- Pass less args than msg requires. +ok, res = pcall(lerror.vshard, lerror.code.MISSING_MASTER) +res:match('bad argument #2.*') +-- Pass more args than `args` field contains. +ok, res = pcall(lerror.vshard, lerror.code.MISSING_MASTER, 'arg1', 'arg2') +res:match('Wrong number of.*') +-- Pass wrong format code. +ok, res = pcall(lerror.vshard, 'Wrong format code', 'arg1', 'arg2') +res:match('Error message format not found.') + function raise_lua_err() assert(false) end ok, err = pcall(raise_lua_err) lerror.make(err) diff --git a/vshard/error.lua b/vshard/error.lua index 8d26d11..59e4fac 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 +-- the expoted by the module `code` dictionary. +-- * msg -- Error message which can use `args` using +-- `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'} + }, + [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, @@ -16,14 +121,35 @@ 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 vshard error. +-- @param code Number, one of error_code constants. +-- @param ... Arguments from `error_message_template` `args` +-- 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 + if format.msg then + ret.message = string.format(format.msg, ...) + else + ret.message = string.format('%s: %s', format.name, ret) + end + ret.type = 'ShardingError' + ret.code = code + ret.name = format.name return ret end @@ -45,70 +171,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..21093e5 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,7 @@ 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) end return nil, err @@ -519,8 +516,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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH 3/3] Add error messages 2018-06-06 15:57 [tarantool-patches] [PATCH 0/3][vshard] Add error messages AKhatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 1/3] Fix test on bucket_transferring error AKhatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 2/3] Refactor error reporting system AKhatskevich @ 2018-06-06 15:57 ` AKhatskevich 2018-06-07 12:22 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-07 12:22 ` [tarantool-patches] Re: [PATCH 0/3][vshard] " Vladislav Shpilevoy 3 siblings, 1 reply; 11+ messages in thread From: AKhatskevich @ 2018-06-06 15:57 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy 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 @@ -113,7 +113,7 @@ vshard.router.call(100, 'read', 'customer_lookup', {1}, {timeout = 1}) code: 9 type: ShardingError name: NO_ROUTE_TO_BUCKET - message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":100}"' + message: Bucket 100 cannot be found. Is rebalancing in progress? ... -- Wait reconfiguration durigin timeout, if a replicaset was not -- found by bucket.destination from WRONG_BUCKET or @@ -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', 'name': 'WRONG_BUCKET'} ... -- diff --git a/test/router/router.result b/test/router/router.result index 6955b59..dab71f2 100644 --- a/test/router/router.result +++ b/test/router/router.result @@ -199,7 +199,7 @@ util.check_error(vshard.router.call, 1, 'read', 'echo', {123}) code: 9 type: ShardingError name: NO_ROUTE_TO_BUCKET - message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":1}"' + message: Bucket 1 cannot be found. Is rebalancing in progress? ... replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err --- @@ -207,7 +207,7 @@ replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err code: 9 type: ShardingError name: NO_ROUTE_TO_BUCKET - message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":1}"' + message: Bucket 1 cannot be found. Is rebalancing in progress? ... vshard.router.bootstrap() --- @@ -321,7 +321,7 @@ replicaset, err = vshard.router.bucket_discovery(0); return err == nil or err code: 9 type: ShardingError name: NO_ROUTE_TO_BUCKET - message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":0}"' + message: Bucket 0 cannot be found. Is rebalancing in progress? ... replicaset, err = vshard.router.bucket_discovery(1); return err == nil or err --- @@ -361,7 +361,7 @@ util.check_error(vshard.router.call, 1, 'write', 'echo', {123}) --- - null - {'bucket_id': 1, 'code': 7, 'type': 'ShardingError', 'destination': '<replicaset_1>', - 'message': 'TRANSFER_IS_IN_PROGRESS: "{\"bucket_id\":1,\"destination\":\"<replicaset_1>\"}"', + 'message': 'Bucket 1 is transferring to replicaset <replicaset_1>', 'name': 'TRANSFER_IS_IN_PROGRESS'} ... test_run:cmd('switch storage_2_a') @@ -435,7 +435,7 @@ vshard.router.route(vshard.consts.DEFAULT_BUCKET_COUNT + 100) code: 9 type: ShardingError name: NO_ROUTE_TO_BUCKET - message: 'NO_ROUTE_TO_BUCKET: "{\"bucket_id\":3100}"' + message: Bucket 3100 cannot be found. Is rebalancing in progress? ... util.check_error(vshard.router.route, 'asdfg') --- @@ -969,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', 'name': 'NON_MASTER', 'message': 'NON_MASTER: - "{\"bucket_id\":1}"'} +- {'code': 2, 'replica_uuid': '1e02ae8a-afc0-4e91-ba34-843a356b8ed7', 'type': 'ShardingError', + 'replicaset_uuid': '<replicaset_2>', 'message': 'Replica 1e02ae8a-afc0-4e91-ba34-843a356b8ed7 + is not a master for replicaset <replicaset_2> anymore', + 'name': 'NON_MASTER'} ... -- 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 aee9477..1258c9b 100644 --- a/test/storage/storage.result +++ b/test/storage/storage.result @@ -595,10 +595,11 @@ vshard.storage.call(100500, 'read', 'customer_lookup', {1}) --- - null - bucket_id: 100500 + reason: Not found code: 1 type: ShardingError name: WRONG_BUCKET - message: 'WRONG_BUCKET: "{\"bucket_id\":100500}"' + message: 'Cannot perform action with bucket 100500, reason: Not found' ... -- -- Test not existing space in bucket data. diff --git a/vshard/error.lua b/vshard/error.lua index 1271d16..8785695 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,7 +54,8 @@ local error_message_template = { }, [9] = { name = 'NO_ROUTE_TO_BUCKET', - args = {'bucket_id', 'unreachable_uuid'} + msg = 'Bucket %d cannot be found. Is rebalancing in progress?', + args = {'bucket_id'} }, [10] = { name = 'NON_EMPTY', diff --git a/vshard/router/init.lua b/vshard/router/init.lua index 95ee8b3..21093e5 100644 --- a/vshard/router/init.lua +++ b/vshard/router/init.lua @@ -96,8 +96,7 @@ 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, - unreachable_uuid) + err = lerror.vshard(lerror.code.NO_ROUTE_TO_BUCKET, bucket_id) end return nil, err 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' + 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) else errcode = lerror.code.WRONG_BUCKET + reason = 'write prohibited' + 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) end assert(not errcode) assert(mode == 'read' and bucket_is_readable(bucket) or mode == 'write' and bucket_is_writable(bucket)) -::finish:: +::wrong_bucket:: + if bucket and bucket.destination then + reason = "bucket 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 +733,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) -- 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] Add error messages 2018-06-06 15:57 ` [tarantool-patches] [PATCH 3/3] Add error messages AKhatskevich @ 2018-06-07 12:22 ` Vladislav Shpilevoy 2018-06-07 16:12 ` Alex Khatskevich 0 siblings, 1 reply; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-06-07 12:22 UTC (permalink / raw) To: tarantool-patches, AKhatskevich 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'. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] Add error messages 2018-06-07 12:22 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-06-07 16:12 ` Alex Khatskevich 2018-06-07 18:44 ` Vladislav Shpilevoy 0 siblings, 1 reply; 11+ messages in thread From: Alex Khatskevich @ 2018-06-07 16:12 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches > 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] Add error messages 2018-06-07 16:12 ` Alex Khatskevich @ 2018-06-07 18:44 ` Vladislav Shpilevoy 0 siblings, 0 replies; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-06-07 18:44 UTC (permalink / raw) To: Alex Khatskevich, tarantool-patches Thanks for the fixes! Finally the errors have become pretty. >> 4. Same as 2. > Fixed along with > "bucket moving to " -> "bucket moving to " What the difference? "bucket moving to" == "bucket moving to". >> >>> 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. You had labeled the success code as error processing. It is not ok. And in the same code you check 'if bucket and ...', but bucket on success is not nil - it is guaranteed. We must optimize the common case. And the bucket is ok most of time. +-- Only WRONG_BUCKET errors use this label. This is wrong. See three lines above - they process a case when the bucket is ok. You can label it ::wrong_bucket::, but then you must separate this code from success path. I refactored this function, you can look at it on the master branch. The patchset is pushed with my fixes. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 0/3][vshard] Add error messages 2018-06-06 15:57 [tarantool-patches] [PATCH 0/3][vshard] Add error messages AKhatskevich ` (2 preceding siblings ...) 2018-06-06 15:57 ` [tarantool-patches] [PATCH 3/3] Add error messages AKhatskevich @ 2018-06-07 12:22 ` Vladislav Shpilevoy 2018-06-07 13:16 ` Alex Khatskevich 3 siblings, 1 reply; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-06-07 12:22 UTC (permalink / raw) To: tarantool-patches, AKhatskevich Hello. Thanks for the patchset! On 06/06/2018 18:57, AKhatskevich wrote: > Branch: https://github.com/tarantool/vshard/tree/kh/gh-100-error-msg-wo-extra-data > Issue: https://github.com/tarantool/vshard/issues/100 > > *** BLURB HERE *** Why do I see this placeholder here? You should write here a short description of the patchset. Please, answer on this email with the description. > > AKhatskevich (3): > Fix test on bucket_transferring error > Refactor error reporting system > Add error messages > > test/misc/check_uuid_on_connect.result | 8 +- > test/router/reroute_wrong_bucket.result | 11 +- > test/router/router.result | 54 +++++--- > test/router/router.test.lua | 4 +- > test/storage/storage.result | 7 +- > test/unit/error.result | 6 +- > test/unit/error.test.lua | 2 +- > vshard/error.lua | 216 +++++++++++++++++++++----------- > vshard/replicaset.lua | 4 +- > vshard/router/init.lua | 10 +- > vshard/storage/init.lua | 39 +++--- > 11 files changed, 231 insertions(+), 130 deletions(-) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 0/3][vshard] Add error messages 2018-06-07 12:22 ` [tarantool-patches] Re: [PATCH 0/3][vshard] " Vladislav Shpilevoy @ 2018-06-07 13:16 ` Alex Khatskevich 0 siblings, 0 replies; 11+ messages in thread From: Alex Khatskevich @ 2018-06-07 13:16 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches > Why do I see this placeholder here? You should write here a short > description of the patchset. Please, answer on this email with the > description. The aim of this patchset is to make error messages more readable for a user. It consists of three commits: 1. Fix test on bucket_transferring error 2. Refactor error reporting system 3. Add error messages ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-06-07 18:44 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-06 15:57 [tarantool-patches] [PATCH 0/3][vshard] Add error messages AKhatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 1/3] Fix test on bucket_transferring error AKhatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 2/3] Refactor error reporting system AKhatskevich 2018-06-07 12:22 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-07 16:13 ` Alex Khatskevich 2018-06-06 15:57 ` [tarantool-patches] [PATCH 3/3] Add error messages AKhatskevich 2018-06-07 12:22 ` [tarantool-patches] " Vladislav Shpilevoy 2018-06-07 16:12 ` Alex Khatskevich 2018-06-07 18:44 ` Vladislav Shpilevoy 2018-06-07 12:22 ` [tarantool-patches] Re: [PATCH 0/3][vshard] " Vladislav Shpilevoy 2018-06-07 13:16 ` Alex Khatskevich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox