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

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

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.

>> 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,

====================