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 82641212C5 for ; Wed, 27 Jun 2018 05:54:23 -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 HoGA8Ka6DQW5 for ; Wed, 27 Jun 2018 05:54:23 -0400 (EDT) Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 BEF172127B for ; Wed, 27 Jun 2018 05:54:22 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Reload reloadable fiber References: <20180614114202.2634-1-avkhatskevich@tarantool.org> <45c2c224-6bfa-c54d-7654-36961a9d1c66@tarantool.org> <0c6f7167-d61d-010e-9ba5-5c08244a5bb9@tarantool.org> From: Vladislav Shpilevoy Message-ID: <527ad322-e091-dcf6-ad74-5de5a4fcc6ac@tarantool.org> Date: Wed, 27 Jun 2018 12:54:19 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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 Hello. Thanks for the fixes! See 4 comments below. On 26/06/2018 15:50, Alex Khatskevich wrote: > Sorry, I forgot to put the new patch here > > commit 4ddab3c47c5963c3138701d89ba3091d5da9848a > Author: AKhatskevich > Date:   Thu Jun 14 14:03:07 2018 +0300 > >     Reload reloadable fiber > >     Fixed a problem: >     The `reloadable_fiber_f` was running an infinite while loop and >     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 1. Again 'where loop'. >     in a return statement doesn't increase a stack size. > >     Extra changes: >      * transfer write "started" message responsibility from caller to >        reloadable fiber > >     Closes #116 > > diff --git a/test/unit/util.result b/test/unit/util.result > new file mode 100644 > index 0000000..dbfcce9 > --- /dev/null > +++ b/test/unit/util.result > @@ -0,0 +1,72 @@ > +test_run = require('test_run').new() > +--- > +... > +fiber = require('fiber') > +--- > +... > +log = require('log') > +--- > +... > +test_util = require('util') > +--- > +... > +util = require('vshard.util') > +--- > +... > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +fake_M = { > +    reloadable_func = nil, > +    module_version = 1, > +}; > +--- > +... > +test_run:cmd("setopt delimiter ''"); > +--- > +- true > +... > +-- Check autoreload on function change during failure. > +fake_M.reloadable_function = function () fake_M.reloadable_function = 42; error('Error happened.') end > +--- > +... > +fib = fiber.create(util.reloadable_fiber_f, fake_M, 'reloadable_function', 'Worker_name') > +--- > +... > +fib:cancel() > +--- > +... > +test_run:grep_log('default', 'Worker_name: reloadable function reloadable_function has changed') 2. 'Has been' changed? > +--- > +- 'Worker_name: reloadable function reloadable_function has changed' > +... > diff --git a/vshard/util.lua b/vshard/util.lua > index bb71318..920f53e 100644 > --- a/vshard/util.lua > +++ b/vshard/util.lua > @@ -19,33 +32,37 @@ local function tuple_extract_key(tuple, parts) > +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) 3. Why is these check and warn here, after an error? >          end >      end > +    -- yield serves two pursoses: > +    --  * makes this fiber cancellable > +    --  * prevents 100% cpu consumption > +    fiber.yield() > +    log.info('%s is reloaded, restarting', worker_name) 4. Why do you print 'is reloaded' even if it just thrown an error and the function is not changed actually? > +    -- luajit drops this frame if next function is called in > +    -- return statement. > +    return M.reloadable_fiber_f(module, func_name, worker_name) >  end