[tarantool-patches] Re: [PATCH 2/4] Refactor reloadable fiber

[Vladislav Shpilevoy]

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
>