[tarantool-patches] Re: [PATCH 3/3] Introduce multiple routers feature

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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.