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

[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 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,
 }
-- 
2.24.3 (Apple Git-128)