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 C5CA12094C for ; Tue, 31 Jul 2018 07:24:11 -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 a8uTOqmhXF65 for ; Tue, 31 Jul 2018 07:24:11 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 132C51FFB0 for ; Tue, 31 Jul 2018 07:24:10 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/4] Refactor reloadable fiber References: From: Alex Khatskevich Message-ID: <03fadaa0-c6a1-bc63-69b6-ccdccc155848@tarantool.org> Date: Tue, 31 Jul 2018 14:24:08 +0300 MIME-Version: 1.0 In-Reply-To: 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 On 30.07.2018 14:55, Vladislav Shpilevoy wrote: > 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. renamed to reloadable_fiber_create. > >> * 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. returned > >> ---- >> -- 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. comment added > > 4. 'Data' is never used. I you need it in later patches, then add it > later. deleted > > 5. Why the function almost is not changed, but her diff is 100%? > Please, fix. Fixed by moving part of the comment to reloadable_fiber_main_loop. > >> +    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. deleted