[Tarantool-patches] [PATCH vshard 4/4] router: support netbox return_raw
Oleg Babin
olegrok at tarantool.org
Wed Feb 9 20:53:47 MSK 2022
Thanks for your patch. Two comments below.
On 09.02.2022 03:32, Vladislav Shpilevoy wrote:
> Netbox return_raw option allows to return remote call result
> without decoding, as an msgpack object. It will be introduced
> after 2.10.0-beta2.
>
> It can be used with router working as a proxy. Pass arguments to
> it in msgpack object and get back another msgpack object by usage
> of return_raw option. This way the router won't decode nor encode
> user's function arguments at all.
>
> Part of #312
> ---
> test/instances/storage.lua | 5 ++
> test/reload_evolution/storage.result | 4 --
> test/router-luatest/router_test.lua | 86 ++++++++++++++++++++++++++++
> test/storage/storage.result | 2 -
> vshard/router/init.lua | 28 ++++++++-
> vshard/storage/init.lua | 15 +++++
> vshard/util.lua | 4 +-
> 7 files changed, 136 insertions(+), 8 deletions(-)
>
> diff --git a/test/instances/storage.lua b/test/instances/storage.lua
> index 2d679ba..4a05d82 100755
> --- a/test/instances/storage.lua
> +++ b/test/instances/storage.lua
> @@ -12,10 +12,15 @@ box.ctl.set_on_shutdown_timeout(0.001)
> box.cfg(helpers.box_cfg())
> box.schema.user.grant('guest', 'super', nil, nil, {if_not_exists = true})
>
> +local function box_error()
> + box.error(box.error.PROC_LUA, 'box_error')
> +end
> +
> local function echo(...)
> return ...
> end
>
> +_G.box_error = box_error
> _G.echo = echo
>
> _G.ready = true
> diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/storage.result
> index 77010a2..a282bba 100644
> --- a/test/reload_evolution/storage.result
> +++ b/test/reload_evolution/storage.result
> @@ -122,8 +122,6 @@ vshard.storage.call(bucket_id_to_move, 'read', 'do_select', {42})
> ---
> - true
> - - [42, 3000]
> -- null
> -- null
> ...
> vshard.storage.bucket_send(bucket_id_to_move, util.replicasets[1])
> ---
> @@ -155,8 +153,6 @@ vshard.storage.call(bucket_id_to_move, 'read', 'do_select', {42})
> ---
> - true
> - - [42, 3000]
> -- null
> -- null
> ...
> -- Check info() does not fail.
> vshard.storage.info() ~= nil
> diff --git a/test/router-luatest/router_test.lua b/test/router-luatest/router_test.lua
> index 13ab74d..23929dc 100644
> --- a/test/router-luatest/router_test.lua
> +++ b/test/router-luatest/router_test.lua
> @@ -99,3 +99,89 @@ g.test_msgpack_args = function(g)
> end
>
> end -- vutil.feature.msgpack_object
> +
> +if vutil.feature.netbox_return_raw then
> +
> +local function test_return_raw_template(g, mode)
> + local router = g.router
> + --
> + -- Normal call.
> + --
> + local res = router:exec(function(timeout, mode)
> + return add_details(vshard.router[mode](1, 'echo', {1, 2, 3},
> + {timeout = timeout, return_raw = true}))
> + end, {wait_timeout, mode})
> + t.assert_equals(res.val, {1, 2, 3}, 'value value')
> + t.assert_equals(res.val_type, 'userdata', 'value type')
> + t.assert(not res.err, 'no error')
> +
> + --
> + -- Route call.
> + --
> + res = router:exec(function(timeout, mode)
> + local route = vshard.router.route(1)
> + return add_details(route[mode](route, 'echo', {1, 2, 3},
> + {timeout = timeout, return_raw = true}))
> + end, {wait_timeout, mode})
> + t.assert_equals(res.val, {1, 2, 3}, 'value value')
> + t.assert_equals(res.val_type, 'userdata', 'value type')
> + t.assert(not res.err, 'no error')
> +
> + --
> + -- Empty result set.
> + --
> + res = router:exec(function(timeout, mode)
> + return add_details(vshard.router[mode](1, 'echo', {},
> + {timeout = timeout, return_raw = true}))
> + end, {wait_timeout, mode})
> + t.assert(not res.val, 'no value')
> + t.assert(not res.err, 'no error')
> +
> + --
> + -- Error.
> + --
> + res = router:exec(function(timeout, mode)
> + return add_details(vshard.router[mode](1, 'box_error', {1, 2, 3},
> + {timeout = timeout}))
> + end, {wait_timeout, mode})
> + t.assert(not res.val, 'no value')
> + t.assert_equals(res.err_type, 'table', 'error type')
> + t.assert_covers(res.err, {type = 'ClientError', code = box.error.PROC_LUA},
> + 'error value')
> +
> + --
> + -- Route error.
> + --
> + res = router:exec(function(timeout, mode)
> + local route = vshard.router.route(1)
> + return add_details(route[mode](route, 'box_error', {1, 2, 3},
> + {timeout = timeout}))
> + end, {wait_timeout, mode})
> + t.assert(not res.val, 'no value')
> + t.assert_equals(res.err_type, 'table', 'error type')
> + t.assert_covers(res.err, {type = 'ClientError', code = box.error.PROC_LUA},
> + 'error value')
> +end
> +
> +g.test_return_raw = function(g)
g.skip_if could be used
Also probably you could use parametrization (however I won't insist):
```
local pg = t.group('pgroup', {{mode = 'callrw'}, {engine = 'callro'}})
pg.test = function(cg)
...
end
```
> + g.router:exec(function()
> + rawset(_G, 'add_details', function(val, err)
> + -- Direct return would turn nils into box.NULLs. The tests want to
> + -- ensure it doesn't happen. Table wrap makes the actual nils
> + -- eliminate themselves.
> + return {
> + val = val,
> + val_type = type(val),
> + err = err,
> + err_type = type(err),
> + }
> + end)
> + end)
> + test_return_raw_template(g, 'callrw')
> + test_return_raw_template(g, 'callro')
> + g.router:exec(function()
> + _G.add_details = nil
> + end)
> +end
> +
> +end -- vutil.feature.netbox_return_raw
> diff --git a/test/storage/storage.result b/test/storage/storage.result
> index dcd1c1f..73c171a 100644
> --- a/test/storage/storage.result
> +++ b/test/storage/storage.result
> @@ -480,8 +480,6 @@ vshard.storage.call(1, 'read', 'space_get', {'test', {1}})
> ---
> - true
> - [1, 1]
> -- null
> -- null
> ...
> vshard.storage.call(100500, 'read', 'space_get', {'test', {1}})
> ---
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 44ed801..d39f489 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -1,7 +1,10 @@
> local log = require('log')
> local lfiber = require('fiber')
> +local lmsgpack = require('msgpack')
> local table_new = require('table.new')
> local fiber_clock = lfiber.clock
> +local msgpack_is_object = lmsgpack.is_object
> +local msgpack_object = lmsgpack.object
I suggest to add something like
```
if msgpack_is_object == nil then
msgpack_is_object = function() error("Msgpack object feature is not
supported by current Tarantool version") end
end
```
>
> local MODULE_INTERNALS = '__module_vshard_router'
> -- Reload requirements, in case this module is reloaded manually.
> @@ -530,14 +533,17 @@ end
> --
> local function router_call_impl(router, bucket_id, mode, prefer_replica,
> balance, func, args, opts)
> + local do_return_raw
> if opts then
> if type(opts) ~= 'table' or
> (opts.timeout and type(opts.timeout) ~= 'number') then
> error('Usage: call(bucket_id, mode, func, args, opts)')
> end
> opts = table.copy(opts)
> - elseif not opts then
> + do_return_raw = opts.return_raw
> + else
> opts = {}
> + do_return_raw = false
> end
> local timeout = opts.timeout or consts.CALL_TIMEOUT_MIN
> local replicaset, err
> @@ -569,6 +575,26 @@ local function router_call_impl(router, bucket_id, mode, prefer_replica,
> local storage_call_status, call_status, call_error =
> replicaset[call](replicaset, 'vshard.storage.call',
> {bucket_id, mode, func, args}, opts)
> + if do_return_raw and msgpack_is_object(storage_call_status) then
> + -- Storage.call returns in the first value a flag whether user's
> + -- function threw an exception or not. Need to extract it.
> + -- Unfortunately, it forces to repack the rest of values into a
> + -- new array. But the values themselves are not decoded.
> + local it = storage_call_status:iterator()
> + local count = it:decode_array_header()
> + storage_call_status = it:decode()
> + -- When no values, nil is not packed into msgpack object. Same
> + -- as in raw netbox.
> + if count > 1 then
> + count = count - 1
> + local res = table_new(count, 0)
> + for i = 1, count do
> + res[i] = it:take()
> + end
> + call_status = msgpack_object(res)
> + end
> + call_error = nil
> + end
> if storage_call_status then
> if call_status == nil and call_error ~= nil then
> return call_status, call_error
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 6820ad0..5083911 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -2512,6 +2512,21 @@ local function storage_call(bucket_id, mode, name, args)
> if not ok then
> ret1 = lerror.make(ret1)
> end
> + -- Truncate nil values. Can't return them all because empty values turn into
> + -- box.NULL. Even if user's function actually returned just 1 value, this
> + -- would lead to '1, box.NULL, box.NULL' on the client. Not visible on the
> + -- router when called normally, but won't help if the router did
> + -- 'return_raw' and just forwards everything as is without truncation.
> + -- Although this solution truncates really returned nils.
> + if ret3 == nil then
> + if ret2 == nil then
> + if ret1 == nil then
> + return ok
> + end
> + return ok, ret1
> + end
> + return ok, ret1, ret2
> + end
> return ok, ret1, ret2, ret3
> end
>
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 72176f7..ad469f2 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -234,8 +234,10 @@ end
> -- Dictionary of supported core features on the given instance. Try to use it
> -- in all the other code rather than direct version check.
> --
> +local is_ge_2_10_0_beta2_86 = version_is_at_least(2, 10, 0, 'beta', 2, 86)
> local feature = {
> - msgpack_object = version_is_at_least(2, 10, 0, 'beta', 2, 86),
> + msgpack_object = is_ge_2_10_0_beta2_86,
> + netbox_return_raw = is_ge_2_10_0_beta2_86,
> }
>
> return {
More information about the Tarantool-patches
mailing list