From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8A0CD287FE for ; Wed, 8 Aug 2018 08:28:32 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9lQU06iJFPKE for ; Wed, 8 Aug 2018 08:28:32 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 456CD287E9 for ; Wed, 8 Aug 2018 08:28:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] Introduce multiple routers feature References: <30ab88fc-fba0-11d8-254c-385e59caead7@tarantool.org> <9a0a958a-5f66-2aa5-83de-e2d6f55cbd71@tarantool.org> <9109c71a-0313-82c8-02db-3e4dd8c833cd@tarantool.org> From: Vladislav Shpilevoy Message-ID: <875d8175-1655-25d3-1ed2-469ca0df596e@tarantool.org> Date: Wed, 8 Aug 2018 15:28:29 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alex Khatskevich , tarantool-patches@freelists.org Thanks for the fixes! 1. Please, rebase on the master. I've failed to do it easy. 2. Please, adding a new commit send it to the same thread. I am talking about "Fix: do not update route map in place". Since you've not sent it, I review it here. 2.1. At first, please, prefix the commit title with a subsystem name the patch is for. Here it is not "Fix: ", but "router: ". 2.2. We know a new route map size before rebuild - it is equal to the total bucket count. So it can be allocated once via table.new(total_bucket_count, 0). It allows to avoid reallocs. I've fixed both remarks and pushed the commit into the master. > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 59c25a0..b31f7dc 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -799,6 +848,90 @@ if M.errinj.ERRINJ_RELOAD then >      error('Error injection: reload') >  end > > +-------------------------------------------------------------------------------- > +-- Managing router instances > +-------------------------------------------------------------------------------- > + > +local function cfg_reconfigure(router, cfg) > +    return router_cfg(router, cfg, false) > +end > + > +local router_mt = { > +    __index = { > +        cfg = cfg_reconfigure; > +        info = router_info; > +        buckets_info = router_buckets_info; > +        call = router_call; > +        callro = router_callro; > +        callrw = router_callrw; > +        route = router_route; > +        routeall = router_routeall; > +        bucket_id = router_bucket_id; > +        bucket_count = router_bucket_count; > +        sync = router_sync; > +        bootstrap = cluster_bootstrap; > +        bucket_discovery = bucket_discovery; > +        discovery_wakeup = discovery_wakeup; > +    } > +} > + > +-- Table which represents this module. > +local module = {} > + > +-- This metatable bypasses calls to a module to the static_router. > +local module_mt = {__index = {}} > +for method_name, method in pairs(router_mt.__index) do > +    module_mt.__index[method_name] = function(...) > +        return method(M.static_router, ...) > +    end > +end > + > +local function export_static_router_attributes() > +    setmetatable(module, module_mt) > +end > + > +-- > +-- Create a new instance of router. > +-- @param name Name of a new router. > +-- @param cfg Configuration for `router_cfg`. > +-- @retval Router instance. > +-- @retval Nil and error object. > +-- > +local function router_new(name, cfg) > +    if type(name) ~= 'string' or type(cfg) ~= 'table' then > +           error('Wrong argument type. Usage: vshard.router.new(name, cfg).') > +    end > +    if M.routers[name] then > +        return nil, lerror.vshard(lerror.code.ROUTER_ALREADY_EXISTS, name) > +    end > +    local router = table.deepcopy(ROUTER_TEMPLATE) > +    setmetatable(router, router_mt) > +    router.name = name > +    M.routers[name] = router > +    router_cfg(router, cfg) 3. router_cfg can raise an error from box.cfg. So on an error lets catch it, remove the router from M.routers and rethrow the error. In other things the patch LGTM. Please, fix the comments above and I will push it. Thank you for working on this!