Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Oleg Babin <olegrok@tarantool.org>, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster
Date: Mon, 4 May 2020 23:09:53 +0200	[thread overview]
Message-ID: <df14f457-a4fa-0dff-afe3-49dc9cbc8a23@tarantool.org> (raw)
In-Reply-To: <2bb5c86a-04c4-96a9-ab3e-75b96d43b1d9@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,

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

  reply	other threads:[~2020-05-04 21:09 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-01  0:16 [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way Vladislav Shpilevoy
2020-05-01 16:58   ` Oleg Babin
2020-05-02 20:08     ` Vladislav Shpilevoy
2020-05-04 14:26       ` Oleg Babin
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 2/7] router: introduce discovery_mode Vladislav Shpilevoy
2020-05-01 16:59   ` Oleg Babin
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 3/7] test: disable router discovery for some tests Vladislav Shpilevoy
2020-05-01 17:00   ` Oleg Babin
2020-05-02 20:09     ` Vladislav Shpilevoy
2020-05-04 14:26       ` Oleg Babin
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 4/7] test: clear route map, respecting statistics Vladislav Shpilevoy
2020-05-01 17:00   ` Oleg Babin
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 5/7] router: keep known bucket count stat up to date Vladislav Shpilevoy
2020-05-01 17:01   ` Oleg Babin
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster Vladislav Shpilevoy
2020-05-01 17:01   ` Oleg Babin
2020-05-02 20:12     ` Vladislav Shpilevoy
2020-05-04 14:26       ` Oleg Babin
2020-05-04 21:09         ` Vladislav Shpilevoy [this message]
2020-05-06  8:27           ` Oleg Babin
2020-05-07 22:45   ` Konstantin Osipov
2020-05-08 19:56     ` Vladislav Shpilevoy
2020-05-09  7:37       ` Konstantin Osipov
2020-05-01  0:16 ` [Tarantool-patches] [PATCH vshard 7/7] router: introduce discovery mode 'once' Vladislav Shpilevoy
2020-05-01 17:02   ` Oleg Babin
2020-05-02 20:12     ` Vladislav Shpilevoy
2020-05-04 14:27       ` Oleg Babin
2020-05-06 20:54 ` [Tarantool-patches] [PATCH vshard 0/7] Router extended discovery Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=df14f457-a4fa-0dff-afe3-49dc9cbc8a23@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH vshard 6/7] router: make discovery smoother in a big cluster' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox