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 D8521267FA for ; Mon, 30 Jul 2018 07:55:37 -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 jCqPhZEtAz3g for ; Mon, 30 Jul 2018 07:55:37 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 92798267B0 for ; Mon, 30 Jul 2018 07:55:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/4] Refactor reloadable fiber References: From: Vladislav Shpilevoy Message-ID: Date: Mon, 30 Jul 2018 14:55:33 +0300 MIME-Version: 1.0 In-Reply-To: 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: tarantool-patches@freelists.org, AKhatskevich Thanks for the patch! See 6 comments below. On 30/07/2018 11:56, AKhatskevich wrote: > Reloadable fiber changes: > * renamed reloadable_fiber_f -> reloadable_fiber 1. Now the name is bad. And not only name is changed - you've changed the semantics. Reloadable_fiber_f was a function to start, but reloadable_fiber is a function that starts itself. And it is not clear from its name. Please, rename it to make clear that it starts the fiber. > * reloadable_fiber creates fiber and gives name on its own > * worker name replaced with a fiber name in logs > * added extra data argument, which is passed to a function > --- > test/rebalancer/rebalancer.result | 2 +- > test/rebalancer/rebalancer.test.lua | 2 +- > test/router/reload.result | 4 +-- > test/router/reload.test.lua | 4 +-- > test/storage/reload.result | 6 ++-- > test/storage/reload.test.lua | 6 ++-- > test/unit/garbage.result | 2 +- > test/unit/garbage.test.lua | 2 +- > test/unit/util.result | 20 ++++--------- > test/unit/util.test.lua | 12 ++++---- > vshard/router/init.lua | 16 +++++----- > vshard/storage/init.lua | 21 +++++++------ > vshard/util.lua | 59 +++++++++++++++++++++---------------- > 13 files changed, 76 insertions(+), 80 deletions(-) >> diff --git a/test/unit/util.result b/test/unit/util.result > index 30906d1..56b863e 100644 > --- a/test/unit/util.result > +++ b/test/unit/util.result > @@ -58,16 +54,12 @@ log.info(string.rep('a', 1000)) > fake_M.reloadable_function = function () fiber.sleep(0.01); return true end > --- > ... > -fib = fiber.create(util.reloadable_fiber_f, fake_M, 'reloadable_function', 'Worker_name') > +fib = util.reloadable_fiber('Worker_name', fake_M, 'reloadable_function') > --- > ... > -while not test_run:grep_log('default', 'Worker_name is reloaded, restarting') do fiber.sleep(0.01) end > +while not test_run:grep_log('default', 'module is reloaded, restarting') do fiber.sleep(0.01) end > --- > ... > fib:cancel() > --- > ... > -test_run:grep_log('default', 'Worker_name has been started', 1000) 2. Why did you remove 'started' message? It is not ok. Please, return back. > ---- > -- Worker_name has been started > -... > diff --git a/vshard/util.lua b/vshard/util.lua > index fb875ce..1319acc 100644 > --- a/vshard/util.lua > +++ b/vshard/util.lua > @@ -31,6 +31,30 @@ local function tuple_extract_key(tuple, parts) > return key > end > > +local function reloadable_fiber_main_loop(module, func_name, data) 3. No any comment. 4. 'Data' is never used. I you need it in later patches, then add it later. 5. Why the function almost is not changed, but her diff is 100%? Please, fix. > + local func = module[func_name] > +::restart_loop:: > + local ok, err = pcall(func, data) > + -- yield serves two purposes: > + -- * makes this fiber cancellable > + -- * prevents 100% cpu consumption > + fiber.yield() > + if not ok then > + log.error('%s has been failed: %s', func_name, err) > + if func == module[func_name] then > + goto restart_loop > + end > + -- There is a chance that error was raised during reload > + -- (or caused by reload). Perform reload in case function > + -- has been changed. > + log.error('reloadable function %s has been changed', func_name) > + end > + log.info('module is reloaded, restarting') > + -- luajit drops this frame if next function is called in > + -- return statement. > + return M.reloadable_fiber_main_loop(module, func_name, data) > +end > + > -- > -- Wrapper to run a func in infinite loop and restart it on > -- errors and module reload. > @@ -44,30 +68,13 @@ end > -- For example: "Garbage Collector", "Recovery", "Discovery", > -- "Rebalancer". Used only for an activity logging. > -- 6. Now the comment is obsolete. > -local function reloadable_fiber_f(module, func_name, worker_name) > +local function reloadable_fiber(worker_name, module, func_name, data) > + assert(type(worker_name) == 'string') > + local xfiber = fiber.create(reloadable_fiber_main_loop, module, func_name, > + data) > log.info('%s has been started', worker_name) > - local func = module[func_name] > -::reload_loop:: > - local ok, err = pcall(func, module.module_version) > - -- yield serves two pursoses: > - -- * makes this fiber cancellable > - -- * prevents 100% cpu consumption > - fiber.yield() > - if not ok then > - log.error('%s has been failed: %s', worker_name, err) > - if func == module[func_name] then > - goto reload_loop > - end > - -- There is a chance that error was raised during reload > - -- (or caused by reload). Perform reload in case function > - -- has been changed. > - log.error('%s: reloadable function %s has been changed', > - worker_name, func_name) > - end > - log.info('%s is reloaded, restarting', worker_name) > - -- luajit drops this frame if next function is called in > - -- return statement. > - return M.reloadable_fiber_f(module, func_name, worker_name) > + xfiber:name(worker_name) > + return xfiber > end >