Re: [Tarantool-patches] [PATCH vshard 2/2] storage: introduce vshard.storage._call()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 22/03/2020 06:08, Oleg Babin wrote:
> Thanks for your patch! See comments bellow.
> 
> On 21/03/2020 21:59, Vladislav Shpilevoy wrote:
>> _call can't be properly integrated at this moment, because that
>> would break rebalancing from old nodes. The new function is only
>> added. Its usage and drop of old functions will happen in 0.1.17
>> when I will finish and merge top commit from this branch:
>> https://github.com/tarantool/vshard/tree/gerold103/gh-227-drop-old-functions
> 
> Could you add some "smoke" tests for "_call" function? The test in this patch only checks that it exists but don't check that it works. If it's hard and non-trivial ignore this comment.

Ok, see my attempt below. It is hard to test each function, because

    1) bucket_recv() requires to be called twice, and with non-trivial
    arguments;

    2) rebalancer_apply_routes() is async, it starts a fiber and returns
    always true, so to test its _call invocation it is necessary to do
    some manual rebalancing.

But since _call functions are anyway used in other places, and are
already tested, the only thing left to test is whether all functions
are added to _call API, and if the arguments are forwarded correctly.

So I added a new function for this: 'test_api'. It returns which
functions are present in _call API, and how arguments are forwarded.
(And which proves why _call is so useful - can add anything to there,
hidden from a user, from _func space, even add some test things.)

====================
diff --git a/test/upgrade/upgrade.result b/test/upgrade/upgrade.result
index 104d31a..9e6d810 100644
--- a/test/upgrade/upgrade.result
+++ b/test/upgrade/upgrade.result
@@ -162,6 +162,16 @@ vshard.storage._call ~= nil
  | ---
  | - true
  | ...
+vshard.storage._call('test_api', 1, 2, 3)
+ | ---
+ | - bucket_recv: true
+ |   rebalancer_apply_routes: true
+ |   test_api: true
+ |   rebalancer_request_state: true
+ | - 1
+ | - 2
+ | - 3
+ | ...
 
 test_run:switch('storage_1_b')
  | ---
diff --git a/test/upgrade/upgrade.test.lua b/test/upgrade/upgrade.test.lua
index 422920e..aea0da8 100644
--- a/test/upgrade/upgrade.test.lua
+++ b/test/upgrade/upgrade.test.lua
@@ -53,6 +53,7 @@ box.space._schema:get({'vshard_version'})
 vshard.storage.internal.schema_current_version()
 vshard.storage.internal.schema_latest_version
 vshard.storage._call ~= nil
+vshard.storage._call('test_api', 1, 2, 3)
 
 test_run:switch('storage_1_b')
 box.space._schema:get({'vshard_version'})
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 77bc9dd..6aa5d6a 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -2184,11 +2184,24 @@ local function storage_call(bucket_id, mode, name, args)
     return ok, ret1, ret2, ret3
 end
 
-local service_call_api = {
+local service_call_api
+
+local function service_call_test_api(...)
+    return service_call_api, ...
+end
+
+service_call_api = setmetatable({
     bucket_recv = bucket_recv,
     rebalancer_apply_routes = rebalancer_apply_routes,
     rebalancer_request_state = rebalancer_request_state,
-}
+    test_api = service_call_test_api,
+}, {__serialize = function(api)
+    local res = {}
+    for k, _ in pairs(api) do
+        res[k] = true
+    end
+    return res
+end})
 
 local function service_call(...)
     local service_name = select(1, ...)
====================

__serialize is redefined so as not to print function
addresses in the test.

>>   +local service_call_api = {
>> +    bucket_recv = bucket_recv,
>> +    rebalancer_apply_routes = rebalancer_apply_routes,
>> +    rebalancer_request_state = rebalancer_request_state,
>> +}
>> +
>> +local function service_call(...)
>> +    local service_name = select(1, ...)
>> +    return service_call_api[service_name](select(2, ...))
>> +end
> 
> What's about checking that "service_call_api[service_name]" exists?

Since the API is entirely internal, I decided to avoid here
such sanity checks against API misusage.