[Tarantool-patches] [PATCH vshard 3/4] router: support msgpack object args
Oleg Babin
olegrok at tarantool.org
Wed Feb 9 20:53:42 MSK 2022
Thanks for your patch. Two comments below.
On 09.02.2022 03:32, Vladislav Shpilevoy wrote:
> Msgpack object allows to represent scalar values and tables with
> them as a raw MessagePack buffer. It will be introduced after
> 2.10.0-beta2.
>
> It can be used in most of the places which used to take arguments
> for further encoding into MessagePack anyway. Including netbox
> API. Usage of msgpack object allows not to decode the buffer if it
> was received from a remote instance for further proxying. It is
> simply memcpy-ed into a next call.
>
> VShard.router almost supported this feature. Only needed to change
> the arguments type check. But even better - simply drop it and
> trust netbox to do the type check.
>
> Part of #312
> ---
> test/instances/router.lua | 1 +
> test/router-luatest/router_test.lua | 47 +++++++++++++++++++++++++++++
> vshard/replicaset.lua | 4 ---
> vshard/util.lua | 9 ++++++
> 4 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/test/instances/router.lua b/test/instances/router.lua
> index ccec6c1..921ac73 100755
> --- a/test/instances/router.lua
> +++ b/test/instances/router.lua
> @@ -1,5 +1,6 @@
> #!/usr/bin/env tarantool
> local helpers = require('test.luatest_helpers')
> +_G.msgpack = require('msgpack')
> -- Do not load entire vshard into the global namespace to catch errors when code
> -- relies on that.
> _G.vshard = {
> diff --git a/test/router-luatest/router_test.lua b/test/router-luatest/router_test.lua
> index 621794a..13ab74d 100644
> --- a/test/router-luatest/router_test.lua
> +++ b/test/router-luatest/router_test.lua
> @@ -1,5 +1,6 @@
> local t = require('luatest')
> local vtest = require('test.luatest_helpers.vtest')
> +local vutil = require('vshard.util')
> local wait_timeout = 120
>
> local g = t.group('router')
> @@ -52,3 +53,49 @@ g.test_basic = function(g)
> t.assert(not err, 'no error')
> t.assert_equals(res, 1, 'good result')
> end
> +
> +if vutil.feature.msgpack_object then
You could use g.skip_if(vutil.feature.msgpack_object) instead of
conditional test declaration.
> +
> +g.test_msgpack_args = function(g)
> + local router = g.router
> + --
> + -- Normal call ro.
> + --
> + local res, err = router:exec(function(timeout)
> + local args = msgpack.object({100})
> + return vshard.router.callrw(1, 'echo', args, {timeout = timeout})
> + end, {wait_timeout})
> + t.assert(not err, 'no error')
> + t.assert_equals(res, 100, 'good result')
> + --
> + -- Normal call rw.
> + --
> + res, err = router:exec(function(timeout)
> + local args = msgpack.object({100})
> + return vshard.router.callro(1, 'echo', args, {timeout = timeout})
> + end, {wait_timeout})
> + t.assert(not err, 'no error')
> + t.assert_equals(res, 100, 'good result')
> + --
> + -- Direct call ro.
> + --
> + res, err = router:exec(function(timeout)
> + local args = msgpack.object({100})
> + local route = vshard.router.route(1)
> + return route:callro('echo', args, {timeout = timeout})
> + end, {wait_timeout})
> + t.assert(err == nil, 'no error')
> + t.assert_equals(res, 100, 'good result')
> + --
> + -- Direct call rw.
> + --
> + res, err = router:exec(function(timeout)
> + local args = msgpack.object({100})
> + local route = vshard.router.route(1)
> + return route:callrw('echo', args, {timeout = timeout})
> + end, {wait_timeout})
> + t.assert(err == nil, 'no error')
> + t.assert_equals(res, 100, 'good result')
> +end
> +
> +end -- vutil.feature.msgpack_object
> diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
> index 8e5526a..16b89aa 100644
> --- a/vshard/replicaset.lua
> +++ b/vshard/replicaset.lua
> @@ -408,8 +408,6 @@ end
> --
> local function replicaset_master_call(replicaset, func, args, opts)
> assert(opts == nil or type(opts) == 'table')
> - assert(type(func) == 'string', 'function name')
> - assert(args == nil or type(args) == 'table', 'function arguments')
> local master = replicaset.master
> if not master then
> opts = opts and table.copy(opts) or {}
> @@ -552,8 +550,6 @@ local function replicaset_template_multicallro(prefer_replica, balance)
>
> return function(replicaset, func, args, opts)
> assert(opts == nil or type(opts) == 'table')
> - assert(type(func) == 'string', 'function name')
> - assert(args == nil or type(args) == 'table', 'function arguments')
> opts = opts and table.copy(opts) or {}
> local timeout = opts.timeout or consts.CALL_TIMEOUT_MAX
> local net_status, storage_status, retval, err, replica
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 9e0212a..72176f7 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -230,6 +230,14 @@ local function fiber_is_self_canceled()
> return not pcall(fiber.testcancel)
> 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 feature = {
> + msgpack_object = version_is_at_least(2, 10, 0, 'beta', 2, 86),
> +}
It could be simplified to `require('msgpack').object ~= nil`
> +
> return {
> tuple_extract_key = tuple_extract_key,
> reloadable_fiber_create = reloadable_fiber_create,
> @@ -241,4 +249,5 @@ return {
> table_minus_yield = table_minus_yield,
> fiber_cond_wait = fiber_cond_wait,
> fiber_is_self_canceled = fiber_is_self_canceled,
> + feature = feature,
> }
More information about the Tarantool-patches
mailing list