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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 {
-- 
2.24.3 (Apple Git-128)