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 3B21B2817D for ; Mon, 6 Aug 2018 13:03:30 -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 GmWBhBs0V_i7 for ; Mon, 6 Aug 2018 13:03:30 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 ECC732817C for ; Mon, 6 Aug 2018 13:03:29 -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> From: Vladislav Shpilevoy Message-ID: <9109c71a-0313-82c8-02db-3e4dd8c833cd@tarantool.org> Date: Mon, 6 Aug 2018 20:03:28 +0300 MIME-Version: 1.0 In-Reply-To: <9a0a958a-5f66-2aa5-83de-e2d6f55cbd71@tarantool.org> 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 patch! See 8 comments below. 1. You did not send a full diff. There are tests only. (In this email I pasted it myself). Please, send a full diff next times. >>> +    if M.routers[name] then >>> +        return nil, string.format('Router with name %s already exists', name) >>> +    end >>> +    local router = table.deepcopy(ROUTER_TEMPLATE) >>> +    setmetatable(router, router_mt) >>> +    router.name = name >>> +    M.routers[name] = router >>> +    if name == STATIC_ROUTER_NAME then >>> +        M.static_router = router >>> +        export_static_router_attributes() >>> +    end >> >> 9. This check can be removed if you move >> export_static_router_attributes call into legacy_cfg. > Butbue to this if, the static router can be configured by > `vshard.box.new(static_router_name)`. 2. It is not ok. A user should not use any internal names like _statis_router to configure it and get. Please, add a new member vshard.router.static that references the statis one. Until cfg is called it is nil. > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 3e127cb..62fdcda 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -25,14 +25,32 @@ local M = rawget(_G, MODULE_INTERNALS) > if not M then > M = { > ---------------- Common module attributes ---------------- > - -- The last passed configuration. > - current_cfg = nil, > errinj = { > ERRINJ_CFG = false, > ERRINJ_FAILOVER_CHANGE_CFG = false, > ERRINJ_RELOAD = false, > ERRINJ_LONG_DISCOVERY = false, > }, > + -- Dictionary, key is router name, value is a router. > + routers = {}, > + -- Router object which can be accessed by old api: > + -- e.g. vshard.router.call(...) > + static_router = nil, > + -- This counter is used to restart background fibers with > + -- new reloaded code. > + module_version = 0, > + collect_lua_garbage_cnt = 0, 3. A comment? > + } > +end > + > +-- > +-- Router object attributes. > +-- > +local ROUTER_TEMPLATE = { > + -- Name of router. > + name = nil, > + -- The last passed configuration. > + current_cfg = nil, > -- Time to outdate old objects on reload. > connection_outdate_delay = nil, > -- Bucket map cache.> @@ -488,8 +505,20 @@ end > -- Configuration > -------------------------------------------------------------------------------- > > -local function router_cfg(cfg) > - local vshard_cfg, box_cfg = lcfg.check(cfg, M.current_cfg) > +local function change_lua_gc_cnt(val) 4. The same. > + assert(M.collect_lua_garbage_cnt >= 0) > + local prev_cnt = M.collect_lua_garbage_cnt > + M.collect_lua_garbage_cnt = M.collect_lua_garbage_cnt + val > + if prev_cnt == 0 and M.collect_lua_garbage_cnt > 0 then > + lua_gc.set_state(true, consts.COLLECT_LUA_GARBAGE_INTERVAL) > + end > + if prev_cnt > 0 and M.collect_lua_garbage_cnt == 0 then > + lua_gc.set_state(false, consts.COLLECT_LUA_GARBAGE_INTERVAL) > + end 5. You know the concrete val in the caller always: 1 or -1. I think it would look simpler if you split this function into separate inc and dec ones. The former checks for prev == 0 and new > 0, the later checks for prev > 0 and new == 0. It is not needed to check both each time. > +end > + > +local function router_cfg(router, cfg) > + local vshard_cfg, box_cfg = lcfg.check(cfg, router.current_cfg) > if not M.replicasets then > log.info('Starting router configuration') > else > @@ -803,6 +839,77 @@ 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) > +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 > + > +local function router_new(name, cfg) 6. A comment? 7. This function should not check for router_name == static one. It just creates a new router and returns it. The caller should set it into M.routers or M.static_router if needed. For a user you expose not this function but a wrapper that calls router_new and sets M.routers. > + 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, string.format('Router with name %s already exists', name) > + end > + local router = table.deepcopy(ROUTER_TEMPLATE) > + setmetatable(router, router_mt) > + router.name = name > + M.routers[name] = router > + if name == STATIC_ROUTER_NAME then > + M.static_router = router > + export_static_router_attributes() > + end > + router_cfg(router, cfg) > + return router > +end > + > @@ -813,28 +920,23 @@ end > if not rawget(_G, MODULE_INTERNALS) then > rawset(_G, MODULE_INTERNALS, M) > else > - router_cfg(M.current_cfg) > + for _, router in pairs(M.routers) do > + router_cfg(router, router.current_cfg) > + setmetatable(router, router_mt) > + end > M.module_version = M.module_version + 1 > end > > M.discovery_f = discovery_f > M.failover_f = failover_f > +M.router_mt = router_mt > +if M.static_router then > + export_static_router_attributes() > +end 8. This is possible on reload only and can be moved into the if above to the reload case processing.