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

[Vladislav Shpilevoy]

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!