Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Alex Khatskevich <avkhatskevich@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 3/3] Introduce multiple routers feature
Date: Wed, 8 Aug 2018 15:28:29 +0300	[thread overview]
Message-ID: <875d8175-1655-25d3-1ed2-469ca0df596e@tarantool.org> (raw)
In-Reply-To: <e98bf0e4-6014-7947-5871-ed802a8b11eb@tarantool.org>

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!

  reply	other threads:[~2018-08-08 12:28 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-31 16:25 [tarantool-patches] [PATCH 0/3] multiple routers AKhatskevich
2018-07-31 16:25 ` [tarantool-patches] [PATCH 1/3] Update only vshard part of a cfg on reload AKhatskevich
2018-08-01 18:43   ` [tarantool-patches] " Vladislav Shpilevoy
2018-08-03 20:03     ` Alex Khatskevich
2018-08-06 17:03       ` Vladislav Shpilevoy
2018-08-07 13:19         ` Alex Khatskevich
2018-08-08 11:17           ` Vladislav Shpilevoy
2018-07-31 16:25 ` [tarantool-patches] [PATCH 2/3] Move lua gc to a dedicated module AKhatskevich
2018-08-01 18:43   ` [tarantool-patches] " Vladislav Shpilevoy
2018-08-03 20:04     ` Alex Khatskevich
2018-08-06 17:03       ` Vladislav Shpilevoy
2018-08-08 11:17       ` Vladislav Shpilevoy
2018-07-31 16:25 ` [tarantool-patches] [PATCH 3/3] Introduce multiple routers feature AKhatskevich
2018-08-01 18:43   ` [tarantool-patches] " Vladislav Shpilevoy
2018-08-03 20:05     ` Alex Khatskevich
2018-08-06 17:03       ` Vladislav Shpilevoy
2018-08-07 13:18         ` Alex Khatskevich
2018-08-08 12:28           ` Vladislav Shpilevoy [this message]
2018-08-08 14:04             ` Alex Khatskevich
2018-08-08 15:37               ` Vladislav Shpilevoy
2018-08-01 14:30 ` [tarantool-patches] [PATCH] Check self arg passed for router objects AKhatskevich
2018-08-03 20:07 ` [tarantool-patches] [PATCH] Refactor config templates AKhatskevich
2018-08-06 15:49   ` [tarantool-patches] " Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875d8175-1655-25d3-1ed2-469ca0df596e@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=avkhatskevich@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 3/3] Introduce multiple routers feature' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox