From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5EA392553B for ; Tue, 19 Jun 2018 07:49:44 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7R_FWQiCm6uS for ; Tue, 19 Jun 2018 07:49:44 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9B4332550A for ; Tue, 19 Jun 2018 07:49:43 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] Complete module reload References: <991c6880-9d5e-3f51-9003-a095984dc30d@tarantool.org> <18667cab-8501-a46d-8255-89abfcb2bf0c@tarantool.org> From: Alex Khatskevich Message-ID: Date: Tue, 19 Jun 2018 14:49:36 +0300 MIME-Version: 1.0 In-Reply-To: <18667cab-8501-a46d-8255-89abfcb2bf0c@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , tarantool-patches@freelists.org > Hello. Thanks for the fixes! See 10 comments below and > the commit with some new fixes on the branch. your fixes are squashed > > 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. > Ok, I like this approach. > 2. You did not outdate replica metatables. How did your tests pass then? Added test for outdate replica object && addrd outdated = true attribute to old replicaset and replica objects. > > 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. > Ok, I like this approach. >> +    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. I have deleted an intermediate function, so, now it looks like this         util.async_task(outdate_delay, outdate_replicasets, old_replicasets) And I suppose, renaming is redundant. >> +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. Done > >> >> @@ -514,20 +579,17 @@ local function buildall(sharding_cfg, >> old_replicasets) > > 6. Unused parameter. Deleted > >> +    outdate_replicasets = outdate_replicasets, > > 7. Why do you need to export this function? Typo. Fixed > >> +    local current_cfg = table.deepcopy(cfg) > > 8. Ok, I understand why do you copy it, but please, > write a comment about it. I decided to make a copy just for box.cfg and use `cfg` where it is needed instead. It makes more sense for me and do not require extra comments. >> -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? > Because M table should not be changed in case of `router_cfg(M.current_cfg)` failure >> + >>   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? Typo. storage do not have such parameter at all. Deleted.