[tarantool-patches] Re: [PATCH 3/3] Add error messages
Alex Khatskevich
avkhatskevich at tarantool.org
Thu Jun 7 19:12:16 MSK 2018
> 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 at 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)
More information about the Tarantool-patches
mailing list