From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 24E7A4696C3 for ; Sun, 22 Mar 2020 22:13:11 +0300 (MSK) References: <1f571d45-179d-1b72-941d-3de21c2e8f11@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4a6edd2a-7173-706c-0622-a805b152dc70@tarantool.org> Date: Sun, 22 Mar 2020 20:13:09 +0100 MIME-Version: 1.0 In-Reply-To: <1f571d45-179d-1b72-941d-3de21c2e8f11@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH vshard 2/2] storage: introduce vshard.storage._call() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Oleg Babin , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@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.