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

[Alex Khatskevich]

On 30.07.2018 14:55, Vladislav Shpilevoy wrote:
> 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.
renamed to reloadable_fiber_create.
>
>> * 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.
returned
>
>> ----
>> -- 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.
comment added
>
> 4. 'Data' is never used. I you need it in later patches, then add it
> later.
deleted
>
> 5. Why the function almost is not changed, but her diff is 100%?
> Please, fix.
Fixed by moving part of the comment to reloadable_fiber_main_loop.
>
>> +    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.
deleted