From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8B15125BD2 for ; Thu, 7 Jun 2018 12:12:20 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id MkaGMKUhdTAv for ; Thu, 7 Jun 2018 12:12:20 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B33B1258E0 for ; Thu, 7 Jun 2018 12:12:19 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] Add error messages References: From: Alex Khatskevich Message-ID: Date: Thu, 7 Jun 2018 19:12:16 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , tarantool-patches@freelists.org > 1. WRONG_BUCKET should not expose 'reason' as an attribute. Discussed verbally. Let it just be exposed and useless and undocumented. > 2. 'is prohibited'. No? I just wanted short error messaged. Fixed. > 3. How could 'errcode' be nil, if you had set it one line above? Thanks > 4. Same as 2. Fixed along with "bucket moving to " -> "bucket moving to " > 5. Same as 3. Fixed > >>       end >>       assert(not errcode) >>       assert(mode == 'read' and bucket_is_readable(bucket) or >>              mode == 'write' and bucket_is_writable(bucket)) >> -::finish:: >> +::wrong_bucket:: > > 6. The code below is executed both on error and on success. But > label is named 'wrong_bucket'. It is not correct. On success > path it is guaranteed that bucket ~= nil. Renamed back to `finish`. IMHO `wrong_bucket` suited well here. > 7. Now msg is set for each error, but you still have checks in > vshard_error like 'if format.msg then'. Fixed New diff: commit 057e640c25dfaab8ccafe3740cddaee27b2c7bc9 Author: AKhatskevich 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': '', -  'name': 'TRANSFER_IS_IN_PROGRESS', 'message': 'TRANSFER_IS_IN_PROGRESS: {"bucket_id":1,"destination":""}'} +  'name': 'TRANSFER_IS_IN_PROGRESS', 'message': 'Bucket 1 is transferring to replicaset +    '}  ...  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': '', +  'type': 'ShardingError', 'message': 'Replica 1e02ae8a-afc0-4e91-ba34-843a356b8ed7 +    is not a master for replicaset 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)