Re: [Tarantool-patches] [PATCH vshard 3/4] router: support msgpack object args

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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,
>   }