From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, AKhatskevich <avkhatskevich@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 2/4] Refactor reloadable fiber Date: Mon, 30 Jul 2018 14:55:33 +0300 [thread overview] Message-ID: <cfb8c77a-d2a5-a3fc-2e07-85957e9414a5@tarantool.org> (raw) In-Reply-To: <aeccbf12a2d1d781323c728eb68caeb948b8d86f.1532940401.git.avkhatskevich@tarantool.org> 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 >
next prev parent reply other threads:[~2018-07-30 11:55 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-30 8:56 [tarantool-patches] [PATCH v4] vshard module reload AKhatskevich 2018-07-30 8:56 ` [tarantool-patches] [PATCH 1/4] Fix races related to object outdating AKhatskevich 2018-07-30 11:55 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-30 16:46 ` Alex Khatskevich 2018-07-30 17:50 ` Vladislav Shpilevoy 2018-07-31 11:05 ` Alex Khatskevich 2018-08-01 12:36 ` Vladislav Shpilevoy 2018-08-01 17:44 ` Alex Khatskevich 2018-08-02 11:51 ` Vladislav Shpilevoy 2018-07-30 8:56 ` [tarantool-patches] [PATCH 2/4] Refactor reloadable fiber AKhatskevich 2018-07-30 11:55 ` Vladislav Shpilevoy [this message] 2018-07-31 11:24 ` [tarantool-patches] " Alex Khatskevich 2018-07-31 11:30 ` Alex Khatskevich 2018-08-01 11:54 ` Vladislav Shpilevoy 2018-07-30 8:56 ` [tarantool-patches] [PATCH 3/4] tests: separate bootstrap routine to a lua_libs AKhatskevich 2018-08-01 12:03 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-30 8:56 ` [tarantool-patches] [PATCH 4/4] Introduce storage reload evolution AKhatskevich 2018-07-30 11:55 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-31 11:29 ` Alex Khatskevich 2018-07-31 11:33 ` Alex Khatskevich 2018-08-01 12:36 ` Vladislav Shpilevoy 2018-08-01 18:09 ` Alex Khatskevich 2018-08-02 11:40 ` Vladislav Shpilevoy 2018-08-02 11:46 ` Vladislav Shpilevoy 2018-08-06 10:59 ` Alex Khatskevich 2018-08-06 15:36 ` Vladislav Shpilevoy 2018-08-06 16:08 ` Alex Khatskevich 2018-08-06 17:18 ` Vladislav Shpilevoy 2018-08-07 9:14 ` Alex Khatskevich 2018-08-08 10:35 ` Vladislav Shpilevoy 2018-08-01 14:07 ` [tarantool-patches] [PATCH] Check self arg passed for router objects AKhatskevich
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cfb8c77a-d2a5-a3fc-2e07-85957e9414a5@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=avkhatskevich@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 2/4] Refactor reloadable fiber' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox