[tarantool-patches] Re: [PATCH 3/3] Introduce destroy module feature

[Vladislav Shpilevoy]

Thanks for the fixes! See 3 comments below.

On 26/06/2018 00:54, Alex Khatskevich wrote:
> 
>>> --- a/test/lua_libs/util.lua
>>> +++ b/test/lua_libs/util.lua
>>> @@ -69,9 +69,43 @@ local function wait_master(test_run, replicaset, master)
>>>       log.info('Slaves are connected to a master "%s"', master)
>>>   end
>>>   +function vshard_fiber_list()
>>> +    -- Flush jit traces to prevent them from
>>> +    -- keeping its upvalues in memory.
>>> +    jit.flush()
>>> +    collectgarbage()
>>> +    -- Give a fiber time to clean itself.
>>> +    fiber.sleep(0.05)
>>
>> 1. Why do you need this sleep? As far as I know
>> collectgarbage just cleans all the garbage in the same
>> fiber with no yields or async tasks.
> netbox: If `collect garbage` has collected a connection, the fiber need a time to wakeup after
> fiber:cancel() to process it and exit.
> another module: the same possible behavior.

1. I removed this sleep and nothing happened. See below comment 5.

> commit b0cddc2a3ba30d3ecf57e71ce8d095bf2f093a5e
> Author: AKhatskevich <avkhatskevich at tarantool.org>
> Date:   Wed Jun 20 00:31:25 2018 +0300
> 
>      Introduce destroy module feature
> 
>      Introduce functions:
>       * vshard.router.destroy()
>       * vshard.storage.destroy()
> 
>      Those functions:
>       * close connections
>       * stop background fibers
>       * delete vshard spaces
>       * delete vshard funcitons

2. Typo.

>       * delete `once` metadate

3. Typo.

> 
>      After the destroy, module can be configured as it was just loaded.
> 
>      Extra changes:
>       * introduce fiber_list function which returns names of non-tarantool
>         fibers
>       * introduce update_M function, which updates M (module internals) with
>         values defined in the module
> 
>      Closes #121
> 
> diff --git a/test/lua_libs/util.lua b/test/lua_libs/util.lua
> index f2d3b48..ce0ea67 100644
> --- a/test/lua_libs/util.lua
> +++ b/test/lua_libs/util.lua
> @@ -69,9 +69,43 @@ local function wait_master(test_run, replicaset, master)
>       log.info('Slaves are connected to a master "%s"', master)
>   end
> 
> +function vshard_fiber_list()
> +    -- Flush jit traces to prevent them from
> +    -- keeping its upvalues in memory.
> +    jit.flush()
> +    collectgarbage()
> +    -- Give a fiber time to clean itself.
> +    fiber.sleep(0.05)

4. It makes no sense not only because it changes nothing in the
test output. Even if it could affect active fiber count because of
fibers that had not managed to exit, you call this vshard_fiber_list()
in the while cycles until its output matches your expectations. So
please, remove the sleep.

5. Since we do not know what to do with spaces cleaning, I will review
the next part of the patch later, when we will know.