[tarantool-patches] Re: [PATCH 3/3] Introduce multiple routers feature
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Aug 6 20:03:28 MSK 2018
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.
More information about the Tarantool-patches
mailing list