From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id F2FB16FC87; Wed, 29 Sep 2021 09:18:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F2FB16FC87 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632896329; bh=J2rqCjlv3mo46QlC4sLe9nZ8vPgcbp7Tm1tQxrmzotA=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=QuevZr+wLXtgxKXs+dCMnI6eFKiArBPBihB22t0KiugExNTGrKmRJzQz1Sq81UNPS cRdyIum64XuskF3QBNGatN83uE2bzSn7eSLmPbeO/1ZELRARK6SWiLyB/UYakruTWH FQKDWafkrXQDkrgve+uJN3zYyfxqreLci9ZUUdwo= Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [94.100.177.90]) (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 DD2236FC87 for ; Wed, 29 Sep 2021 09:18:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DD2236FC87 Received: by smtp30.i.mail.ru with esmtpa (envelope-from ) id 1mVSvV-0006Yb-3c; Wed, 29 Sep 2021 09:18:45 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org References: Message-ID: <6f2babce-b5eb-ecd3-394b-29486e56dbdb@tarantool.org> Date: Wed, 29 Sep 2021 09:18:44 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD96A58C36AA2E99649BF631F26B0465AFD0E15652C7D51B98D182A05F5380850400D417A989C6CF3FA96D45EB04E00A089BCDF60C9CF6D6254390E1485AFA18E4B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72F22E6DC541F75D9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006376B6794A086D6ADA58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89E7EEA36FAB57C81C0E201ACD758C5E6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE4B6963042765DA4B07FB45A5F6E725C8D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE317119E5299B287EE9735652A29929C6CC4224003CC8364762F668D4DB69AB668E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637A5C471A62FCE759D731C566533BA786AA5CC5B56E945C8DA X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5051689B80F6990DB26E32FF58497A9DD1B X-C1DE0DAB: 0D63561A33F958A5215C11E249B3A2827FB863AE4673164F59614718207B4838D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75BFC02AB3DF06BA5A410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34060C3C6DE316ECE4CC936B40A8D9EA8E653232DFFA5A1E59183185428391C8B35F84D09D251097F71D7E09C32AA3244C37AC482106D9708705FAD2C0B6A7042FA8CE788DE6831205729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojWaDhU1ub98aPsLDLVGEMJg== X-Mailru-Sender: 583F1D7ACE8F49BD1042885CEC987B6B7594D0797EB0334596D45EB04E00A08921401CF93FD6FA557019711D9D5B048E1458020726E2BC9FD5ECBA0B92C0A936CDC7563AA7CEBD287402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH vshard 1/2] router: wrap is_async futures completely X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Oleg Babin via Tarantool-patches Reply-To: Oleg Babin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for your patch! I left three comments below. On 29.09.2021 02:08, Vladislav Shpilevoy wrote: > Router used to override :result() method of netbox futures. It is > needed because user functions are called via vshard.storage.call() > which returns some metadata - it must be truncated before > returning the user's data. > > It worked fine while netbox futures were implemented as tables. > But in the newest Tarantool most of netbox state machine code is > moved into C. The futures now are cdata. AFAIK userdata (not cdata) if it makes sense. > They allow to add new members, but can't override their methods. > As a result, on the newest Tarantool is_async in > vshard.router.call() simply didn't work. > > The patch wraps netbox futures completely with a Lua table, not > just overrides one method. Now it works the same on all Tarantool > versions starting from 1.10. > > Closes #294 > --- > test/lua_libs/storage_template.lua | 11 ++ > test/router/router.result | 301 ++++++++++++++++++++++++++++- > test/router/router.test.lua | 120 +++++++++++- > vshard/router/init.lua | 148 ++++++++++---- > 4 files changed, 542 insertions(+), 38 deletions(-) > > diff --git a/test/lua_libs/storage_template.lua b/test/lua_libs/storage_template.lua > index 8df89f6..83ea710 100644 > --- a/test/lua_libs/storage_template.lua ... > + > -- > -- Test errors from router call. > -- > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 3d468f3..42de740 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -470,6 +470,114 @@ end > -- API > -------------------------------------------------------------------------------- > I thing following changes could be moved into sepearate file. > +local function vshard_future_tostring(self) > + return 'vshard.net.box.request' > +end > + > +local function vshard_future_serialize(self) > + -- Drop the metatable. It is also copied and if returned as is leads to > + -- recursive serialization. > + local s = setmetatable(table.deepcopy(self), {}) > + s._base = nil > + return s > +end > + > +local function vshard_future_is_ready(self) > + return self._base:is_ready() > +end > + > +local function vshard_future_wrap_result(res) > + local storage_ok, res, err = unpack(res) Unpack could be replaced with ... = res[1], res[2], res[3]. Should be a bit faster since unpack can't be jitted. > + if storage_ok then > + if res == nil and err ~= nil then > + return nil, lerror.make(err) > + end > + return setmetatable({res}, seq_serializer) > + end > + return nil, lerror.make(res) > +end > + > +local function vshard_future_result(self) > + local res, err = self._base:result() > + if res == nil then > + return nil, lerror.make(err) > + end > + return vshard_future_wrap_result(res) > +end > + > +local function vshard_future_wait_result(self, timeout) > + local res, err = self._base:wait_result(timeout) > + if res == nil then > + return nil, lerror.make(err) > + end > + return vshard_future_wrap_result(res) > +end > + > +local function vshard_future_discard(self) > + return self._base:discard() > +end > + > +local function vshard_future_iter_next(iter, i) > + local res, err > + local base_next = iter.base_next > + local base_req = iter.base_req > + local base = iter.base > + -- Need to distinguish the last response from the pushes. Because the former > + -- has metadata returned by vshard.storage.call(). > + -- At the same time there is no way to check if the base pairs() did its > + -- last iteration except calling its next() function again. > + -- This, in turn, might lead to a block if the result is not ready yet. > + i, res = base_next(base, i) > + -- To avoid that there is a 2-phase check. > + -- If the request isn't finished after first next(), it means the result is > + -- not received. This is a push. Return as is. > + -- If the request is finished, it is safe to call next() again to check if > + -- it ended. It won't block. > + local is_done = base_req:is_ready() > + > + if not is_done then > + -- Definitely a push. It would be finished if the final result was > + -- received. > + if i == nil then > + return nil, lerror.make(res) > + end > + return i, res > + end > + if i == nil then > + if res ~= nil then > + return i, lerror.make(res) > + end > + return nil, nil > + end > + -- Will not block because the request is already finished. > + if base_next(base, i) == nil then > + res, err = vshard_future_wrap_result(res) > + if res ~= nil then > + return i, res > + end > + return i, {nil, lerror.make(err)} > + end > + return i, res > +end > + > +local function vshard_future_pairs(self, timeout) > + local next_f, iter, i = self._base:pairs(timeout) > + return vshard_future_iter_next, > + {base = iter, base_req = self, base_next = next_f}, i > +end > + > +local vshard_future_mt = { > + __tostring = vshard_future_tostring, > + __serialize = vshard_future_serialize, > + __index = { > + is_ready = vshard_future_is_ready, > + result = vshard_future_result, > + wait_result = vshard_future_wait_result, > + discard = vshard_future_discard, > + pairs = vshard_future_pairs, > + } > +} > + > -- > -- Since 1.10 netbox supports flag 'is_async'. Given this flag, a > -- request result is returned immediately in a form of a future > @@ -482,41 +590,9 @@ end > -- So vshard.router.call should wrap a future object with its own > -- unpacker of result. > -- > --- Unpack a result given from a future object from > --- vshard.storage.call. If future returns [status, result, ...] > --- this function returns [result]. Or a classical couple > --- nil, error. > --- > -function future_storage_call_result(self) > - local res, err = self:base_result() > - if not res then > - return nil, err > - end > - local storage_call_status, call_status, call_error = unpack(res) > - if storage_call_status then > - if call_status == nil and call_error ~= nil then > - return call_status, call_error > - else > - return setmetatable({call_status}, seq_serializer) > - end > - end > - return nil, call_status > -end > - > --- > --- Given a netbox future object, redefine its 'result' method. > --- It is impossible to just create a new signle metatable per > --- the module as a copy of original future's one because it has > --- some upvalues related to the netbox connection. > --- > -local function wrap_storage_call_future(future) > - -- Base 'result' below is got from __index metatable under the > - -- hood. But __index is used only when a table has no such a > - -- member in itself. So via adding 'result' as a member to a > - -- future object its __index.result can be redefined. > - future.base_result = future.result > - future.result = future_storage_call_result > - return future > +local function vshard_future_new(future) > + -- Use '_' as a prefix so as users could use all normal names. > + return setmetatable({_base = future}, vshard_future_mt) > end > > -- Perform shard operation > @@ -574,7 +650,7 @@ local function router_call_impl(router, bucket_id, mode, prefer_replica, > -- values: true/false and func result. But > -- async returns future object. No true/false > -- nor func result. So return the first value. > - return wrap_storage_call_future(storage_call_status) > + return vshard_future_new(storage_call_status) > end > end > err = lerror.make(call_status)