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 4FA6C28757 for ; Wed, 1 Aug 2018 14:43:41 -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 GTATOD6RQbeC for ; Wed, 1 Aug 2018 14:43:41 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 D7A6428754 for ; Wed, 1 Aug 2018 14:43:40 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] Introduce multiple routers feature References: From: Vladislav Shpilevoy Message-ID: <30ab88fc-fba0-11d8-254c-385e59caead7@tarantool.org> Date: Wed, 1 Aug 2018 21:43:37 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, AKhatskevich Thanks for the patch! See 10 comments below. On 31/07/2018 19:25, AKhatskevich wrote: > Key points: > * Old `vshard.router.some_method()` api is preserved. > * Add `vshard.router.new(name, cfg)` method which returns a new router. > * Each router has its own: > 1. name > 2. background fibers > 3. attributes (route_map, replicasets, outdate_delay...) > * Module reload reloads all configured routers. > * `cfg` reconfigures a single router. > * All routers share the same box configuration. The last passed config > overrides the global config. > * Multiple router instances can be connected to the same cluster. > * By now, a router cannot be destroyed. > > Extra changes: > * Add `data` parameter to `reloadable_fiber_create` function. > > Closes #130 > ---> diff --git a/test/multiple_routers/multiple_routers.result b/test/multiple_routers/multiple_routers.result > new file mode 100644 > index 0000000..33f4034 > --- /dev/null > +++ b/test/multiple_routers/multiple_routers.result > @@ -0,0 +1,226 @@ > +-- Reconfigure one of routers do not affect the others. > +routers[3]:cfg(configs.cfg_1) 1. You did not change configs.cfg_1 so it is not reconfig actually. Please, change something to check that the parameter affects one router and does not affect others. 2. Please, add a test on an ability to get the static router into a variable and use it like others. It should be possible to hide distinctions between static and other routers. Like this: r1 = vshard.router.static r2 = vshard.router.new(...) do_something_with_router(r1) do_something_with_router(r2) Here do_something_with_router() is unaware of whether the router is static or not. > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index 3e127cb..7569baf 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -257,13 +272,13 @@ local function router_call(bucket_id, mode, func, args, opts) > -- but already is executed on storages. > while lfiber.time() <= tend do > lfiber.sleep(0.05) > - replicaset = M.replicasets[err.destination] > + replicaset = router.replicasets[err.destination] > if replicaset then > goto replicaset_is_found > end > end > else > - replicaset = bucket_set(bucket_id, replicaset.uuid) > + replicaset = bucket_set(router, bucket_id, replicaset.uuid) 3. Out of 80 symbols. > lfiber.yield() > -- Protect against infinite cycle in a > -- case of broken cluster, when a bucket > @@ -488,9 +503,14 @@ end > -- Configuration > -------------------------------------------------------------------------------- > > -local function router_cfg(cfg) > - local vshard_cfg, box_cfg = lcfg.check(cfg, M.current_cfg) > - if not M.replicasets then > +-- Types of configuration. > +CFG_NEW = 'new' > +CFG_RELOAD = 'reload' > +CFG_RECONFIGURE = 'reconfigure' 4. Last two values are never used in router_cfg(). The first is used for logging only and can be checked as it was before with no explicit passing. > + > +local function router_cfg(router, cfg, cfg_type) > + local vshard_cfg, box_cfg = lcfg.check(cfg, router.current_cfg) > + if cfg_type == CFG_NEW then > log.info('Starting router configuration') > else > log.info('Starting router reconfiguration') > @@ -512,44 +532,53 @@ local function router_cfg(cfg) > + > +local function updage_lua_gc_state() 5. This function is not needed actually. On router_new() the only thing that can change is start of the gc fiber if the new router has the flag and the gc is not started now. It can be checked by a simple 'if' with no full-scan of all routers. On reload it is not possible to change configuration, so the gc state can not be changed and does not need an update. Even if it could be changed, you already iterate over routers on reload to call router_cfg and can collect their flags along side. The next point is that it is not possible now to manage the gc via simple :cfg() call. You do nothing with gc when router_cfg is called directly. And that produces a question - why do your tests pass if so? The possible solution - keep a counter of set lua gc flags overall routers in M. On each cfg you update the counter if the value is changed. If it was 0 and become > 0, then you start gc. If it was > 0 and become 0, then you stop gc. No routers iteration at all. > + local lua_gc = false > + for _, xrouter in pairs(M.routers) do > + if xrouter.collect_lua_garbage then > + lua_gc = true > + end > + end > + lua_gc.set_state(lua_gc, consts.COLLECT_LUA_GARBAGE_INTERVAL) > end > > @@ -803,6 +833,93 @@ 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, CFG_RECONFIGURE) > +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 = {} > + > +local function export_static_router_attributes() > + -- 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(...) > + if M.static_router then > + return method(M.static_router, ...) > + else > + error('Static router is not configured') 6. This should not be all-time check. You should initialize the static router metatable with only errors. On the first cfg you reset the metatable to always use regular methods. But anyway this code is unreachable. See below in the comment 10 why it is so. > + end > + end > + end > + setmetatable(module, module_mt) > + -- Make static_router attributes accessible form > + -- vshard.router.internal. > + local M_static_router_attributes = { > + name = true, > + replicasets = true, > + route_map = true, > + total_bucket_count = true, > + } 7. I saw in the tests that you are using vshard.router.internal.static_router instead. Please, remove M_static_router_attributes then. > + setmetatable(M, { > + __index = function(M, key) > + return M.static_router[key] > + end > + }) > +end > + > +local function router_new(name, cfg) > + assert(type(name) == 'string' and type(cfg) == 'table', > + 'Wrong argument type. Usage: vshard.router.new(name, cfg).') 8. As I said before, do not use assertions for usage checks in public API. Use 'if wrong_usage then error(...) 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 9. This check can be removed if you move export_static_router_attributes call into legacy_cfg. 10. Looks like all your struggles in export_static_router_attributes() about error on non-configured router makes no sense since until cfg is called, vshard.router has no any methods except cfg and new. > + router_cfg(router, cfg, CFG_NEW) > + updage_lua_gc_state() > + return router > +end > +