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 changes. LGTM.

On 11.02.2022 01:33, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
>>> 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
>>> @@ -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.
> Looks better indeed:
>
> ====================
> @@ -54,13 +54,12 @@ g.test_basic = function(g)
>       t.assert_equals(res, 1, 'good result')
>   end
>   
> -if vutil.feature.msgpack_object then
> -
>   g.test_msgpack_args = function(g)
> -    local router = g.router
> +    t.run_only_if(vutil.feature.msgpack_object)
>       --
>       -- Normal call ro.
>       --
> +    local router = g.router
>       local res, err = router:exec(function(timeout)
>           local args = msgpack.object({100})
>           return vshard.router.callrw(1, 'echo', args, {timeout = timeout})
> @@ -97,5 +96,3 @@ g.test_msgpack_args = function(g)
>       t.assert(err == nil, 'no error')
>       t.assert_equals(res, 100, 'good result')
>   end
> -
> -end -- vutil.feature.msgpack_object
> ====================
>
>>> 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`
> Thanks, looks good:
>
> ====================
> @@ -3,6 +3,7 @@ local log = require('log')
>   local fiber = require('fiber')
>   local lerror = require('vshard.error')
>   local lversion = require('vshard.version')
> +local lmsgpack = require('msgpack')
>   
>   local MODULE_INTERNALS = '__module_vshard_util'
>   local M = rawget(_G, MODULE_INTERNALS)
> @@ -235,7 +236,7 @@ end
>   -- in all the other code rather than direct version check.
>   --
>   local feature = {
> -    msgpack_object = version_is_at_least(2, 10, 0, 'beta', 2, 86),
> +    msgpack_object = lmsgpack.object ~= nil,
>   
> ====================
>
>
> New patch:
>
> ====================
> router: support msgpack object args
>
> 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 | 44 +++++++++++++++++++++++++++++
>   vshard/replicaset.lua               |  4 ---
>   vshard/util.lua                     | 10 +++++++
>   4 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/test/instances/router.lua b/test/instances/router.lua
> index 587a473..288a052 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..b7aeea9 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,46 @@ g.test_basic = function(g)
>       t.assert(not err, 'no error')
>       t.assert_equals(res, 1, 'good result')
>   end
> +
> +g.test_msgpack_args = function(g)
> +    t.run_only_if(vutil.feature.msgpack_object)
> +    --
> +    -- Normal call ro.
> +    --
> +    local router = g.router
> +    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
> 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..354727d 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -3,6 +3,7 @@ local log = require('log')
>   local fiber = require('fiber')
>   local lerror = require('vshard.error')
>   local lversion = require('vshard.version')
> +local lmsgpack = require('msgpack')
>   
>   local MODULE_INTERNALS = '__module_vshard_util'
>   local M = rawget(_G, MODULE_INTERNALS)
> @@ -230,6 +231,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 = lmsgpack.object ~= nil,
> +}
> +
>   return {
>       tuple_extract_key = tuple_extract_key,
>       reloadable_fiber_create = reloadable_fiber_create,
> @@ -241,4 +250,5 @@ return {
>       table_minus_yield = table_minus_yield,
>       fiber_cond_wait = fiber_cond_wait,
>       fiber_is_self_canceled = fiber_is_self_canceled,
> +    feature = feature,
>   }