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 2/2] Complete module reload
Date: Tue, 19 Jun 2018 12:00:42 +0300	[thread overview]
Message-ID: <18667cab-8501-a46d-8255-89abfcb2bf0c@tarantool.org> (raw)
In-Reply-To: <d542133d-b62e-0237-f062-5875fba5cbb4@tarantool.org>

Hello. Thanks for the fixes! See 10 comments below and
the commit with some new fixes on the branch.

> diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
> index 99f59aa..4e7bc10 100644
> --- a/vshard/replicaset.lua
> +++ b/vshard/replicaset.lua
> @@ -338,26 +339,80 @@ local function replicaset_tostring(replicaset)
>   end
> 
>   --
> --- Rebind connections of old replicas to new ones.
> +-- Outdate old objects in case of reload/reconfigure.
>   --
> -local function replicaset_rebind_connections(replicaset)
> -    for _, replica in pairs(replicaset.replicas) do
> -        local old_replica = replica.old_replica
> -        if old_replica then
> -            local conn = old_replica.conn
> -            replica.conn = conn
> -            replica.down_ts = old_replica.down_ts
> -            replica.net_timeout = old_replica.net_timeout
> -            replica.net_sequential_ok = old_replica.net_sequential_ok
> -            replica.net_sequential_fail = old_replica.net_sequential_fail
> -            if conn then
> -                conn.replica = replica
> -                conn.replicaset = replicaset
> -                old_replica.conn = nil
> +local function cleanup_old_objects(old_replicaset_mt, old_replica_mt,
> +                                 old_replicas)
> +    local function get_outdated_warning()
> +        return function(...)
> +            return nil, lerror.vshard(lerror.code.OBJECT_IS_OUTDATED)
> +        end
> +    end

1. Please, declare this outdated_mt out of this function in a module
local variable like it is done with ordinal replica/replicaset mt. Not just
__index, but entire mt. See comment 3 why.

2. You did not outdate replica metatables. How did your tests pass then?

> +    old_replicaset_mt.__index = get_outdated_warning
> +    -- Leave replica_mt unchanged. All its methods are safe.
> +    for _, replica in pairs(old_replicas) do
> +        replica.conn = nil
> +    end
> +    log.info('Old replicaset and replica objects are outdated.')
> +end
> +
> +--
> +-- Initiate outdating process, which cancels connections and
> +-- spoils object methods.
> +--
> +local function outdate_replicasets(replicasets, outdate_delay)
> +    -- Collect data to outdate old objects.
> +    local old_replicaset_mt = nil
> +    local old_replica_mt = nil
> +    local old_replicas = {}
> +    for _, replicaset in pairs(replicasets) do
> +        local old_mt = getmetatable(replicaset)
> +        assert(old_replicaset_mt == nil or old_replicaset_mt == old_mt)
> +        old_replicaset_mt = old_mt
> +        for _, replica in pairs(replicaset.replicas) do
> +            table.insert(old_replicas, replica)
> +            local old_mt = getmetatable(replica)
> +            assert(old_replica_mt == nil or old_replica_mt == old_mt)
> +            old_replica_mt = old_mt
> +        end
> +    end
> +    util.async_task(outdate_delay, cleanup_old_objects, old_replicaset_mt,
> +                    old_replica_mt, old_replicas)
3. You do not need to search old_replica_mt and old_replicaset_mt here. You do not
need them at all. Just declare special object_outdate_mt and set this metatable to
the old objects. Not just __index, but entire metatable. So you can get rid of
this old mts search, old mt parameters.

> +end
> +
> +
> +--
> +-- Copy netbox conections from old replica objects to new ones
> +-- and outdate old objects.
> +-- @param replicasets New replicasets
> +-- @param old_replicasets Replicasets and replicas to be outdated.
> +-- @param outdate_delay Number of seconds; delay to outdate
> +--        old objects.
> +--
> +local function rebind_replicasets(replicasets, old_replicasets, outdate_delay)
> +    for replicaset_uuid, replicaset in pairs(replicasets) do
> +        local old_replicaset = old_replicasets and
> + old_replicasets[replicaset_uuid]> +        for replica_uuid, replica in pairs(replicaset.replicas) do
> +            local old_replica = old_replicaset and
> + old_replicaset.replicas[replica_uuid]> +            if old_replica then
> +                local conn = old_replica.conn
> +                replica.conn = conn
> +                replica.down_ts = old_replica.down_ts
> +                replica.net_timeout = old_replica.net_timeout
> +                replica.net_sequential_ok = old_replica.net_sequential_ok
> +                replica.net_sequential_fail = old_replica.net_sequential_fail
> +                if conn then
> +                    conn.replica = replica
> +                    conn.replicaset = replicaset
> +                end
>               end
> -            replica.old_replica = nil
>           end
>       end
> +    if old_replicasets then
> +        outdate_replicasets(old_replicasets, outdate_delay)

4. Please, name it schedule_connections_outdate. Now the name
confuses. It could be thought to deprecate the objects now, not
after timeout.

> +    end
>   end
> @@ -406,11 +452,30 @@ local replica_mt = {
>           end
>       end,
>   }
> -index = {}
> -for name, func in pairs(replica_mt.__index) do
> -    index[name] = gsc("replica", name, replica_mt, func)
> +
> +--
> +-- Get new copy of replicaset and replica meta tables.
> +-- It is important to have distinct copies of meta tables to
> +-- be able to outdate only old objects.
> +--
> +local function copy_metatables()

5. Please, remove. This function is harmful and slow. You do not
need to copy metatables. Use my recommendations above and think.

> +    --
> +    -- Wrap self methods with a sanity checker.
> +    --
> +    local replicaset_mt_copy = table.copy(replicaset_mt)
> +    local replica_mt_copy = table.copy(replica_mt)
> +    local index = {}
> +    for name, func in pairs(replicaset_mt_copy.__index) do
> +        index[name] = gsc("replicaset", name, replicaset_mt_copy, func)
> +    end
> +    replicaset_mt_copy.__index = index
> +    index = {}
> +    for name, func in pairs(replica_mt_copy.__index) do
> +        index[name] = gsc("replica", name, replica_mt_copy, func)
> +    end
> +    replica_mt_copy.__index = index
> +    return replicaset_mt_copy, replica_mt_copy
>   end
> -replica_mt.__index = index
> 
> @@ -514,20 +579,17 @@ local function buildall(sharding_cfg, old_replicasets)

6. Unused parameter.

>           zone_weights = {}
>       end
> @@ -535,8 +597,8 @@ local function buildall(sharding_cfg, old_replicasets)
>                   uri = replica.uri, name = replica.name, uuid = replica_uuid,
>                   zone = replica.zone, net_timeout = consts.CALL_TIMEOUT_MIN,
>                   net_sequential_ok = 0, net_sequential_fail = 0,
> -                down_ts = curr_ts, old_replica = old_replica,
> -            }, replica_mt)
> +                down_ts = curr_ts,
> +            }, replica_mt_copy)
>               new_replicaset.replicas[replica_uuid] = new_replica
>               if replica.master then
>                   new_replicaset.master = new_replica
> @@ -596,4 +658,6 @@ return {
>       buildall = buildall,
>       calculate_etalon_balance = cluster_calculate_etalon_balance,
>       wait_masters_connect = wait_masters_connect,
> +    rebind_replicasets = rebind_replicasets,
> +    outdate_replicasets = outdate_replicasets,

7. Why do you need to export this function?

>   }
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 1dee80c..d70d060 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -459,6 +473,7 @@ end
> 
>   local function router_cfg(cfg)
>       cfg = lcfg.check(cfg)
> +    local current_cfg = table.deepcopy(cfg)

8. Ok, I understand why do you copy it, but please,
write a comment about it.

>       if not M.replicasets then
>           log.info('Starting router configuration')
>       else
> @@ -776,15 +792,16 @@ end
>   -- About functions, saved in M, and reloading see comment in
>   -- storage/init.lua.
>   --
> -M.discovery_f = discovery_f
> -M.failover_f = failover_f
> -
> -if not rawget(_G, '__module_vshard_router') then
> -    rawset(_G, '__module_vshard_router', M)
> +if not rawget(_G, MODULE_INTERNALS) then
> +    rawset(_G, MODULE_INTERNALS, M)
>   else
> +    router_cfg(M.current_cfg)
>       M.module_version = M.module_version + 1
>   end
> 
> +M.discovery_f = discovery_f
> +M.failover_f = failover_f

9. Why did you move these lines?

> +
>   return {
>       cfg = router_cfg;
>       info = router_info;
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index 879c7c4..50347b0 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -21,6 +33,8 @@ if not M then
>           -- See format in replicaset.lua.
>           --
>           replicasets = nil,
> +        -- Time to deprecate old objects on reload.
> +        reload_deprecate_timeout = nil,

10. Why here is 'reload_deprecate_timeout', but 'connection_outdate_delay'
in router?

>           -- Triggers on master switch event. They are called right
>           -- before the event occurs.
>           _on_master_enable = trigger.new('_on_master_enable'),

  reply	other threads:[~2018-06-19  9:00 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-09 17:47 [tarantool-patches] [PATCH 0/2][vshard] Vshard safe reload AKhatskevich
2018-06-09 17:47 ` [tarantool-patches] [PATCH 1/2] Add test on error during reconfigure AKhatskevich
2018-06-17 19:46   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-18 10:10     ` Alex Khatskevich
2018-06-18 10:48       ` Vladislav Shpilevoy
2018-06-19 10:36         ` Alex Khatskevich
2018-06-09 17:47 ` [tarantool-patches] [PATCH 2/2] Complete module reload AKhatskevich
2018-06-17 19:46   ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-19  7:09     ` Alex Khatskevich
2018-06-19  9:00       ` Vladislav Shpilevoy [this message]
2018-06-19 11:49         ` Alex Khatskevich
2018-06-19 11:57           ` Vladislav Shpilevoy
2018-06-17 19:39 ` [tarantool-patches] Re: [PATCH 0/2][vshard] Vshard safe reload Vladislav Shpilevoy
2018-06-19  7:20   ` Alex Khatskevich

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=18667cab-8501-a46d-8255-89abfcb2bf0c@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=avkhatskevich@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 2/2] Complete module reload' \
    /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