Tarantool development patches archive
 help / color / mirror / Atom feed
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
>   

  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