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 A111827097 for ; Thu, 21 Jun 2018 08:04:57 -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 p9W8u4Pt9Cd3 for ; Thu, 21 Jun 2018 08:04:57 -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 1D89D2708C for ; Thu, 21 Jun 2018 08:04:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Reload reloadable fiber References: <20180614114202.2634-1-avkhatskevich@tarantool.org> From: Vladislav Shpilevoy Message-ID: <45c2c224-6bfa-c54d-7654-36961a9d1c66@tarantool.org> Date: Thu, 21 Jun 2018 15:04:53 +0300 MIME-Version: 1.0 In-Reply-To: <20180614114202.2634-1-avkhatskevich@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: AKhatskevich , tarantool-patches@freelists.org Hello. Thanks for the patch! See my 6 comments below. On 14/06/2018 14:42, AKhatskevich wrote: > Fixed a problem: > The `reloadable_fiber_f` was running an infinite where loop and 1. What is a 'where loop'? > preventing the whole module from being reloaded. > > This behavior is fixed by calling new version of `reloadable_fiber_f` in > a return statement instead of the where loop. Note: calling a function > in a return statement doesn't increase a stack size. > > Closes #116 > --- > Branch: https://github.com/tarantool/vshard/tree/kh/gh-116-reloadable > Issue: https://github.com/tarantool/vshard/issues/116 > > test/router/reload.result | 4 +-- > test/router/reload.test.lua | 4 +-- > test/storage/reload.result | 6 ++-- > test/storage/reload.test.lua | 6 ++-- > vshard/util.lua | 78 +++++++++++++++++++++++++++++++------------- > 5 files changed, 65 insertions(+), 33 deletions(-) > > diff --git a/test/router/reload.result b/test/router/reload.result > index 19a9ead..47f3c2e 100644 > --- a/test/router/reload.result > +++ b/test/router/reload.result > @@ -116,10 +116,10 @@ vshard.router.module_version() > check_reloaded() > --- > ... > -while test_run:grep_log('router_1', 'Failover has been reloaded') == nil do fiber.sleep(0.1) end > +while test_run:grep_log('router_1', 'Failover has been started') == nil do fiber.sleep(0.1) end 2. Why? Please, leave the old message. Router already writes that failover is started in router_cfg. In other places the same. > diff --git a/vshard/util.lua b/vshard/util.lua > index bb71318..fa51701 100644 > --- a/vshard/util.lua > +++ b/vshard/util.lua > @@ -2,6 +2,24 @@ > local log = require('log') > local fiber = require('fiber') > > +local MODULE_INTERNALS = '__module_vshard_util' > +local M = rawget(_G, MODULE_INTERNALS) > +if not M then > + -- > + -- The module is loaded for the first time. > + -- > + M = { > + -- Latest versions of functions. > + reloadable_fiber_f = nil, > + errinj = { > + RELOADABLE_STACK_MAX = nil, 3. What is the point of this error injection? It tests Lua, not VShard, as I think. And it takes too many lines in the reloadable fiber function complicating its understanding. So lets remove. > + RELOADABLE_EXIT = nil, > + } > + > + } > + rawset(_G, MODULE_INTERNALS, M) > +end > + > @@ -19,33 +37,43 @@ local function tuple_extract_key(tuple, parts) > end > > -- > --- Wrapper to run @a func in infinite loop and restart it on the > --- module reload. This function CAN NOT BE AUTORELOADED. To update > --- it you must manualy stop all fibers, run by this function, do > --- reload, and then restart all stopped fibers. This can be done, > --- for example, by calling vshard.storage/router.cfg() again with > --- the same config as earlier. > +-- Wrapper to run a func in infinite loop and restart it on > +-- errors and module reload. > +-- To handle module reload and run new version of a function > +-- in the module, the function should just return. > -- > --- @param func Reloadable function to run. It must accept current > --- module version as an argument, and interrupt itself, > --- when it is changed. > --- @param worker_name Name of the function. Usual infinite fiber > --- represents a background subsystem, which has a name. For > --- example: "Garbage Collector", "Recovery", "Discovery", > --- "Rebalancer". > --- @param M Module which can reload. > +-- @param module Module which can be reloaded. > +-- @param func_name Name of a function to be executed in the > +-- module. > +-- @param worker_name Name of the reloadable background subsystem. > +-- For example: "Garbage Collector", "Recovery", "Discovery", > +-- "Rebalancer". Used only for an activity logging. > -- > -local function reloadable_fiber_f(M, func_name, worker_name) > - while true do > - local ok, err = pcall(M[func_name], M.module_version) > - if not ok then > - log.error('%s has been failed: %s', worker_name, err) > - fiber.yield() > - else > - log.info('%s has been reloaded', worker_name) > - fiber.yield() > +local function reloadable_fiber_f(module, func_name, worker_name) > + log.info('%s has been started', worker_name) > + local func = module[func_name] > + local ok, err = pcall(func, module.module_version) > + if not ok then > + log.error('%s has been failed: %s', worker_name, err) > + if func ~= module[func_name] then > + log.warn('%s reloadable function %s has changed', > + worker_name, func_name) > end > end > + fiber.yield() > + log.info('%s is reloading', worker_name) > + if M.errinj.RELOADABLE_EXIT then > + return 4. How is this error possible? There are no lines in reloadable_fiber_f that can terminate the fiber. 5. Now on any reload I see two messages: started reloading started reloading But actually the fiber is started once. Please, return the old messages. > diff --git a/test/unit/util.result b/test/unit/util.result > new file mode 100644 > index 0000000..ea9edfa > --- /dev/null > +++ b/test/unit/util.result > @@ -0,0 +1,107 @@ > +test_run = require('test_run').new() > +--- > +... > +util = require('vshard.util') > +--- > +... > +test_util = require('util') 6. Unused variable? > +--- > +... > +log = require('log') > +--- > +...