Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster

[Oleg Babin <olegrok@tarantool.org>]

Thanks for your answers.

On 05/05/2020 00:09, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
>>>>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>>>>> index 6d88153..26ea85b 100644
>>>>> --- a/vshard/router/init.lua
>>>>> +++ b/vshard/router/init.lua
>>>>> @@ -250,50 +250,127 @@ if util.version_is_at_least(1, 10, 0) then
>>>>> +            -- Don't spam many requests at once. Give
>>>>> +            -- storages time to handle them and other
>>>>> +            -- requests.
>>>>> +            lfiber.sleep(consts.DISCOVERY_WORK_STEP)
>>>>> +            if module_version ~= M.module_version then
>>>>> +                return
>>>>>                 end
>>>>
>>>>
>>>> Is it possible to place such checks to some "common" places. At start/end of each iteration or between steps. This looks strange and a bit unobvous to do such checks after each timeout.
>>>
>>> Purpose is to stop the fiber as soon as possible in case a reload
>>> happened. So moving it to a common place is not really possible.
>>> It would make it work longer with the old code after reload. But
>>> if we keep the checks, I still need to do then after/before any
>>> long yield.
>>>
>>
>> My main concern is that such things could be easily missed in future. But I agree that we should left this function early as possible.
> 
> It can be, and it was a couple of times. Don't know what to do with that
> honestly, at this moment. This restart-on-reload machinery should be reworked
> completely.
> 

I see two ways:
   * An external fiber that periodically checks module reload and reload 
needed fibers.
   * package.preload hook that does the same. (However I not completely 
sure that it's possible)
But anyway I agree it's a separate task and such comments shouldn't 
affect this patchset.

>>> During working on your comments I found that during aggressive discovery I
>>> added sleep in a wrong place. Fixed below:
>>>
>>> diff --git a/vshard/consts.lua b/vshard/consts.lua
>>> index 5391c0f..a6a8c1b 100644
>>> --- a/vshard/consts.lua
>>> +++ b/vshard/consts.lua
>>> @@ -40,5 +40,8 @@ return {
>>>        DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
>>>        RECOVERY_INTERVAL = 5;
>>>        COLLECT_LUA_GARBAGE_INTERVAL = 100;
>>> -    DISCOVERY_INTERVAL = 10;
>>> +    DISCOVERY_IDLE_INTERVAL = 10,
>>> +    DISCOVERY_WORK_INTERVAL = 1,
>>> +    DISCOVERY_WORK_STEP = 0.01,
>>> +    DISCOVERY_TIMEOUT = 10,
>>>    }
>>
>> Nit: here commas and semicolons are mixed. I think we should support it in consistent state - only commas or only semicolons.
> 
> I don't like these ';'. They are against our code style. They were
> added in the second commit in this repository, and since then kept
> growing. I deliberately stopped using them and decided to switch to
> ',' in all new code, and wash out the old lines gradually.
> 
> I guess I can split DISCOVERY_* parameters with an empty line from the others
> who still use ';' though. To make it more clear.
> 
> ====================
> diff --git a/vshard/consts.lua b/vshard/consts.lua
> index a6a8c1b..8c2a8b0 100644
> --- a/vshard/consts.lua
> +++ b/vshard/consts.lua
> @@ -40,6 +40,7 @@ return {
>       DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL = 0.5;
>       RECOVERY_INTERVAL = 5;
>       COLLECT_LUA_GARBAGE_INTERVAL = 100;
> +
>       DISCOVERY_IDLE_INTERVAL = 10,
>       DISCOVERY_WORK_INTERVAL = 1,
>       DISCOVERY_WORK_STEP = 0.01,
> 
> ====================

Ok. I agree. So I don't have comments for this patchset anymore. Feel 
free to push it or ask somebody for additional review :)