Re: [Tarantool-patches] [PATCH vshard 4/4] router: support netbox return_raw

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

Thanks for your fixes. LGTM.

On 11.02.2022 01:34, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
>>> 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
> <...>
>
>>> +    --
>>> +    -- 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
> Done during interactive rebase, diff wasn't saved.
>
>> 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
>>
>> ```
> I tried, but then realized it will affect all the tests in the group.
> If I add in the future more tests which won't need both callro and callrw,
> they will run 2 times without need.
>
> If I make 2 groups - one with 2 modes and other without a config, then
> AFAIU I will need to recreate the cluster when the first group ends and the
> second one starts. I am trying to avoid that because it takes too a lot of
> time (compared to running the test cases).
>
>>> 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
>>
>> ```
> Looks good to me:
>
> ====================
> @@ -3,8 +3,6 @@ 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
>   
>   local MODULE_INTERNALS = '__module_vshard_router'
>   -- Reload requirements, in case this module is reloaded manually.
> @@ -25,6 +23,20 @@ local lreplicaset = require('vshard.replicaset')
>   local util = require('vshard.util')
>   local seq_serializer = { __serialize = 'seq' }
>   
> +local msgpack_is_object = lmsgpack.is_object
> +local msgpack_object = lmsgpack.object
> +
> +if not util.feature.msgpack_object then
> +    local msg = 'Msgpack object feature is not supported by current '..
> +                'Tarantool version'
> +    msgpack_is_object = function()
> +        error(msg)
> +    end
> +    msgpack_object = function()
> +        error(msg)
> +    end
> +end
> +
>   local M = rawget(_G, MODULE_INTERNALS)
> ====================
>
>
> New patch:
>
> ====================
> router: support netbox return_raw
>
> 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  | 84 ++++++++++++++++++++++++++++
>   test/storage/storage.result          |  2 -
>   vshard/router/init.lua               | 40 ++++++++++++-
>   vshard/storage/init.lua              | 15 +++++
>   vshard/util.lua                      |  1 +
>   7 files changed, 144 insertions(+), 7 deletions(-)
>
> diff --git a/test/instances/storage.lua b/test/instances/storage.lua
> index 7ad2af3..2afc942 100755
> --- a/test/instances/storage.lua
> +++ b/test/instances/storage.lua
> @@ -14,10 +14,15 @@ end
>   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 b7aeea9..39dedcb 100644
> --- a/test/router-luatest/router_test.lua
> +++ b/test/router-luatest/router_test.lua
> @@ -96,3 +96,87 @@ g.test_msgpack_args = function(g)
>       t.assert(err == nil, 'no error')
>       t.assert_equals(res, 100, 'good result')
>   end
> +
> +local function test_return_raw_template(g, mode)
> +    --
> +    -- Normal call.
> +    --
> +    local router = g.router
> +    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)
> +    t.run_only_if(vutil.feature.netbox_return_raw)
> +
> +    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
> 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..8f0f489 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -1,5 +1,6 @@
>   local log = require('log')
>   local lfiber = require('fiber')
> +local lmsgpack = require('msgpack')
>   local table_new = require('table.new')
>   local fiber_clock = lfiber.clock
>   
> @@ -22,6 +23,20 @@ local lreplicaset = require('vshard.replicaset')
>   local util = require('vshard.util')
>   local seq_serializer = { __serialize = 'seq' }
>   
> +local msgpack_is_object = lmsgpack.is_object
> +local msgpack_object = lmsgpack.object
> +
> +if not util.feature.msgpack_object then
> +    local msg = 'Msgpack object feature is not supported by current '..
> +                'Tarantool version'
> +    msgpack_is_object = function()
> +        error(msg)
> +    end
> +    msgpack_object = function()
> +        error(msg)
> +    end
> +end
> +
>   local M = rawget(_G, MODULE_INTERNALS)
>   if not M then
>       M = {
> @@ -530,14 +545,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 +587,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 354727d..c6975cf 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -237,6 +237,7 @@ end
>   --
>   local feature = {
>       msgpack_object = lmsgpack.object ~= nil,
> +    netbox_return_raw = version_is_at_least(2, 10, 0, 'beta', 2, 86),
>   }
>   
>   return {