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 8DBD827A0A for ; Thu, 19 Jul 2018 16:32:14 -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 7pEYQ-KIbcbi for ; Thu, 19 Jul 2018 16:32:14 -0400 (EDT) Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 4485421D4C for ; Thu, 19 Jul 2018 16:32:14 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/3] Complete module reload References: <80d76a63-7435-4148-e0ce-37ed2daee19b@tarantool.org> From: Alex Khatskevich Message-ID: <7c9af9eb-7fe9-e07c-2589-56fc192e6614@tarantool.org> Date: Thu, 19 Jul 2018 23:32:05 +0300 MIME-Version: 1.0 In-Reply-To: <80d76a63-7435-4148-e0ce-37ed2daee19b@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 >> >> Part of 112 > > 1. Stray ''. Ok > >> --- >>   test/router/reload.result    | 159 >> +++++++++++++++++++++++++++++++++++++++++++ >>   test/router/reload.test.lua  |  48 +++++++++++++ >>   test/router/router.result    |   3 +- >>   test/storage/reload.result   |  68 ++++++++++++++++++ >>   test/storage/reload.test.lua |  23 +++++++ >>   vshard/cfg.lua               |   6 ++ >>   vshard/error.lua             |   5 ++ >>   vshard/replicaset.lua        | 100 ++++++++++++++++++++------- >>   vshard/router/init.lua       |  44 ++++++++---- >>   vshard/storage/init.lua      |  45 ++++++++---- >>   vshard/util.lua              |  20 ++++++ >>   11 files changed, 466 insertions(+), 55 deletions(-) >> >> diff --git a/test/router/reload.result b/test/router/reload.result >> index 47f3c2e..3fbbe6e 100644 >> --- a/test/router/reload.result >> +++ b/test/router/reload.result >> @@ -174,6 +174,165 @@ vshard.router.module_version() >> ... >> +-- Make sure outdate async task has had cpu time. >> +fiber.sleep(0.005) > > 2. As I asked earlier, please, avoid constant timeouts. > When you want to wait for something, use 'while'. Fixed >> +--- >> +... >> +vshard.router = require('vshard.router') > > 3. You have already did it few lines above. fixed >> +-- Error during reconfigure process. > > 4. You added this test in the previous commit. Thanks > >> +vshard.storage.internal.errinj.ERRINJ_CFG = true > > 5. Same as 4. We have already added the test in the previous > commit, it is not? thanks >> >> diff --git a/vshard/cfg.lua b/vshard/cfg.lua >> index d5429af..87d0fc8 100644 >> --- a/vshard/cfg.lua >> +++ b/vshard/cfg.lua >> @@ -217,6 +217,10 @@ local cfg_template = { >>           type = 'non-negative number', name = 'Sync timeout', >> is_optional = true, >>           default = consts.DEFAULT_SYNC_TIMEOUT >>       }}, >> +    {'connection_outdate_delay', { >> +        type = 'non-negative number', name = 'Object outdate timeout', >> +        is_optional = true, default = nil > > 6. default = nil makes no sense for Lua tables. deleted > >> +    }}, >>   } >>     -- >> @@ -264,6 +268,8 @@ local function remove_non_box_options(cfg) >>       cfg.collect_bucket_garbage_interval = nil >>       cfg.collect_lua_garbage = nil >>       cfg.sync_timeout = nil >> +    cfg.sync_timeout = nil > > 7. Why do you need the second nullify of sync_timeout? rebase consecuence... > >> +    cfg.connection_outdate_delay = nil >>   end >>     return { >> diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua >> index 99f59aa..ec6e95b 100644 >> --- a/vshard/replicaset.lua >> +++ b/vshard/replicaset.lua> @@ -412,6 +424,49 @@ for name, func in >> pairs(replica_mt.__index) do >>   end >>   replica_mt.__index = index >>   +-- >> +-- Meta-methods of outdated objects > > 8. Put the dot at the end of the sentence. fixed > >> +-- They define only arrtibutes from corresponding metatables to > > 9. Typo: arrtibutes. fixed > >> +-- make user able to access fields of old objects. >> +-- >> +local function outdated_warning(...) >> +    return nil, lerror.vshard(lerror.code.OBJECT_IS_OUTDATED) >> +end >> + >> +local outdated_replicaset_mt = { >> +    __index = { >> +        outdated = true >> +    } >> +} > > 10. outdated_replicaset/replica_mt are identical. Please, > make one mt object names 'outdated_mt'. > >> +for fname, func in pairs(replicaset_mt.__index) do >> +    outdated_replicaset_mt.__index[fname] = outdated_warning >> +end > > 11. As I remember, my proposal was to do not > duplicate each method here, but just on any __index return > callable object that on call invokes outdated_warning. > > You can ask, what to do with 'outdated' attribute then, but > you can set it directly in the object in outdate_replicasets() > function. > > What is more, please, use 'is_outdated' since this value is a > flag. And add it to the description on top of the file, where > all attributes are documented. > > Here you can put 'replica.is_outdated = true'. > > Here you can put 'replicaset.is_outdated = true'. 10, 11 - to be discussed. There are two reasons to do it that way: 1. Replacing data objects with funcitons-wrappers do not make any sense, because it is not called finaly. It just leads to a strange error if it indexed somewhere. 2. Strange error messages appear in bg fibers logs sometimes because of reason 1. > >> +    end >> +    log.info('Old replicaset and replica objects are outdated.') >> +end >> + >>   -- >>   -- Calculate for each replicaset its etalon bucket count. >>   -- Iterative algorithm is used to learn the best balance in a >> diff --git a/vshard/router/init.lua b/vshard/router/init.lua >> index a143070..a8f6bbc 100644 >> --- a/vshard/router/init.lua >> +++ b/vshard/router/init.lua >> @@ -479,12 +493,13 @@ local function router_cfg(cfg) >>       else >>           log.info('Starting router reconfiguration') >>       end >> -    local new_replicasets = lreplicaset.buildall(cfg, M.replicasets) >> +    local new_replicasets = lreplicaset.buildall(cfg) >>       local total_bucket_count = cfg.bucket_count >>       local collect_lua_garbage = cfg.collect_lua_garbage >> -    lcfg.remove_non_box_options(cfg) >> +    local box_cfg = table.deepcopy(cfg) > > 12. As I remember, I asked to use table.copy when > possible. And this case looks appropriate. ok >> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua >> index 052e94f..bf560e6 100644 >> --- a/vshard/storage/init.lua >> +++ b/vshard/storage/init.lua >> @@ -2,20 +2,34 @@ local log = require('log') >>   local luri = require('uri') >>   local lfiber = require('fiber') >>   local netbox = require('net.box') -- for net.box:self() >> +local trigger = require('internal.trigger') >> + >> +local MODULE_INTERNALS = '__module_vshard_storage' >> +-- Reload requirements, in case this module is reloaded manually. >> +if rawget(_G, MODULE_INTERNALS) then >> +    local vshard_modules = { >> +        'vshard.consts', 'vshard.error', 'vshard.cfg', >> +        'vshard.replicaset', 'vshard.util', >> +    } >> +    for _, module in pairs(vshard_modules) do >> +        package.loaded[module] = nil >> +    end >> +end >>   local consts = require('vshard.consts') >>   local lerror = require('vshard.error') >> -local util = require('vshard.util') >>   local lcfg = require('vshard.cfg') >>   local lreplicaset = require('vshard.replicaset') >> -local trigger = require('internal.trigger') >> +local util = require('vshard.util') >>   -local M = rawget(_G, '__module_vshard_storage') >> +local M = rawget(_G, MODULE_INTERNALS) >>   if not M then >>       -- >>       -- The module is loaded for the first time. >>       -- >>       M = { >>           ---------------- Common module attributes ---------------- >> +        -- The last passed configuration. >> +        current_cfg = nil, > > 13. Please, add the same assignment to the router module > initialization. added