[tarantool-patches] Re: [PATCH][vshard] Reload reloadable fiber

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 <avkhatskevich@tarantool.org>
> 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