Tarantool development patches archive
 help / color / mirror / Atom feed
From: Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org,
	yaroslav.dynnikov@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH vshard 5/6] router: introduce automatic master discovery
Date: Mon, 5 Jul 2021 12:24:07 +0300	[thread overview]
Message-ID: <a78915a3-43b0-bf97-0269-ae4c5fa9016f@tarantool.org> (raw)
In-Reply-To: <900c73e7-9824-ad3c-88f8-aa07b0382986@tarantool.org>

Thanks for your answers. See my answers below.

On 03.07.2021 00:35, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
>>> This is getting especially tricky with built-in automatic master
>>> elections which are not supported by vshard yet, but they will be,
>>> they won't depend on any config. Master role won't be pinned to
>>> one node then.
>> Is it possible to implement some simple test that shows if RAFT changes
>>
>> a leader, vshard catches this changes? Of course, it will be test for new Tarantool versions.
> Nope. Because the storages do not support it yet. 'Master' attribute
> can only be set via the config now, unfortunately. This will change
> in the future, but it is blocked by one of the bugs (about bucket refs
> being useless on replicas).

Thanks for answer.

>>> diff --git a/vshard/error.lua b/vshard/error.lua
>>> index bcbcd71..e2d1a31 100644
>>> --- a/vshard/error.lua
>>> +++ b/vshard/error.lua
>>> @@ -153,6 +153,11 @@ local error_message_template = {
>>>            name = 'BUCKET_RECV_DATA_ERROR',
>>>            msg = 'Can not receive the bucket %s data in space "%s" at tuple %s: %s',
>>>            args = {'bucket_id', 'space', 'tuple', 'reason'},
>>> +    },
>>> +    [31] = {
>>> +        name = 'MULTIPLE_MASTERS_FOUND',
>>> +        msg = 'Found more than one master in replicaset %s on nodes %s and %s',
>>> +        args = {'replicaset_uuid', 'master1', 'master2'},
>>>        }
>>>    }
>> Is it possible to test this error?
> It is tested, yes. See master_discovery.test.lua. Section
> "-- Multiple masters logging". Although I couldn't find an
> easy way to expose it into vshard.router.info(). So it is
> simply logged now.

Yes, thanks. I've missed it.


>>>    diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
>>> index fa048c9..7747258 100644
>>> --- a/vshard/replicaset.lua
>>> +++ b/vshard/replicaset.lua
>>> @@ -345,18 +377,30 @@ local function replicaset_master_call(replicaset, func, args, opts)
>>>        assert(opts == nil or type(opts) == 'table')
>>>        assert(type(func) == 'string', 'function name')
>>>        assert(args == nil or type(args) == 'table', 'function arguments')
>>> -    local conn, err = replicaset_connect_master(replicaset)
>>> -    if not conn then
>>> -        return nil, err
>>> -    end
>>> -    if not opts then
>>> -        opts = {timeout = replicaset.master.net_timeout}
>>> -    elseif not opts.timeout then
>>> -        opts = table.copy(opts)
>>> -        opts.timeout = replicaset.master.net_timeout
>>> +    local master = replicaset.master
>>> +    if not master then
>>> +        opts = opts and table.copy(opts) or {}
>>> +        if opts.is_async then
>>> +            return nil, lerror.vshard(lerror.code.MISSING_MASTER,
>>> +                                      replicaset.uuid)
>>> +        end
>> Could / should we here wakeup master discovery if "auto" is specified?
> We can. But I am afraid that under considerable RW requests load we
> might wakeup it too often. And if there are many routers, they might
> create a storm of `vshard.storage._call('info')` requests putting a
> notable load on the storages. So I decided it is not safe until the
> subscriptions are implemented.

But we can't simply fail async requests when master is unknown.

And we even don't have exposed 
replicaset_wait_connected/replicaset_wait_master

to call it before master will be known. As result several seconds we 
will get failed requests.

Also I saw TODO in test that "RO requests should be able to go to 
replicas" and I expect questions

about RTO 0 on read requests from our customers.


I don't know will this feature be used by cartridge but it could create 
small inconvenience for our customers

in test because they should inject timeouts until master will be 
discovered (it's too artifical example but there were

some similar questions when discovery started to work by chunks).

>>> @@ -425,11 +469,6 @@ local function replicaset_template_multicallro(prefer_replica, balance)
>>>                    return r
>>>                end
>>>            end
>>> -        local conn, err = replicaset_connect_master(replicaset)
>>> -        if not conn then
>>> -            return nil, err
>>> -        end
>>> -        return master
>>>        end
>> Why don't we need this part anymore?
> Now the caller handles the case when couldn't find any node. See
> the next diff hunk. I moved it outside, because I didn't want
> pick_next_replica() to take a timeout argument. I wanted it to
> be as simple as possible.

Ok. Thanks for answer.


>>>        return function(replicaset, func, args, opts)
>>> @@ -444,9 +483,13 @@ local function replicaset_template_multicallro(prefer_replica, balance)
>>>            end
>>>            local end_time = fiber_clock() + timeout
>>>            while not net_status and timeout > 0 do
>>> -            replica, err = pick_next_replica(replicaset)
>>> +            replica = pick_next_replica(replicaset)
>>>                if not replica then
>>> -                return nil, err
>>> +                replica, timeout = replicaset_wait_master(replicaset, timeout)
>>> +                if not replica then
>>> +                    return nil, timeout
>>> +                end
>>> +                replicaset_connect_to_replica(replicaset, replica)
>>>                end
>>>                opts.timeout = timeout
>>>                net_status, storage_status, retval, err =
>>> @@ -508,7 +551,128 @@ local function rebind_replicasets(replicasets, old_replicasets)
> <...>
>
>>> +    local master_uuid
>>> +    for replica_uuid, f in pairs(futures) do
>>> +        if timeout < 0 then
>>> +            -- Netbox uses cond var inside, whose wait throws an error when gets
>>> +            -- a negative timeout. Avoid that.
>>> +            timeout = 0
>>> +        end
>>> +        res, err = f:wait_result(timeout)
>>> +        timeout = deadline - fiber_clock()
>>> +        if not res then
>>> +            f:discard()
>>> +            last_err = err
>>> +            goto next_wait
>> If timeout will be negative we anyway go to next_wait and turn it to 0 at the next iteration.
> Yup. This is intentional. With zero timeout the worst what can happen is a
> yield. But on the other hand, while we were waiting for one request, the
> others could complete and one of them could be the master. This is why I
> continue the iteration - don't want to miss any arrived response.
Ok. Thanks for answer.
> <...>
>
>>> +
>>> +local function locate_masters(replicasets)
>>> +    local is_all_done = true
>>> +    local is_all_nop = true
>>> +    local last_err
>>> +    for _, replicaset in pairs(replicasets) do
>>> +        local is_done, is_nop, err = replicaset_locate_master(replicaset)
>> I think we should log result of master discovery. Especially if error occurred.
> It is logged by the caller. See master_search_f(), init.lua:1067.

Hmm... Yes, but I think such approach with last_err could lose some errors.

E.g. replicaset.lua:616 gets an error, saves it to `last_err` then if 
some error

happened on furure `wait_result` we override `conn.call` error and 
externally we have

a case when there was no any conn.call error.

>>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>>> index 5e2a96b..9407ccd 100644
>>> --- a/vshard/router/init.lua
>>> +++ b/vshard/router/init.lua
>>> @@ -1030,6 +1032,93 @@ local function failover_f(router)
> <...>
>
>>> +--
>>> +-- Master discovery background function. It is supposed to notice master changes
>>> +-- and find new masters in the replicasets, which are configured for that.
>>> +--
>>> +-- XXX: due to polling the search might notice master change not right when it
>> Is this TODO related tohttps://github.com/tarantool/vshard/issues/284  ?
> Yes, that is one of the "solutions", although I would rather call it a crutch.
> I prefer to fix it by doing fair subscriptions. And for that I at least need a
> way to get netbox call result asynchronously, without having to call wait_result()
> anywhere.

Does it request some changes in netbox/iproto? I mean 
https://github.com/tarantool/tarantool/issues/6107


> <...>
>
>>> +local function master_search_set(router)
>>> +    local enable = false
>>> +    for _, rs in pairs(router.replicasets) do
>>> +        if rs.is_auto_master then
>>> +            enable = true
>>> +            break
>>> +        end
>>> +    end
>>> +    local search_fiber = router.master_search_fiber
>>> +    if enable and search_fiber == nil then
>>> +        log.info('Master auto search is enabled')
>>> +        router.master_search_fiber = util.reloadable_fiber_create(
>>> +            'vshard.master_search.' .. router.name, M, 'master_search_f',
>>> +            router)
>> On 1.10 fiber name is limited with 32 symbols.
>>
>> tarantool> #'vshard.master_search.'
>> ---
>> - 21
>> ...
>>
>> That mean that router name is limited with 11 chars.
>>
>> It could be a problem. As I see you don't use {truncate = true} when assign a name
>>
>> https://github.com/tarantool/vshard/blob/a394e3f4008a7436f011c81028dcaadc270eebf6/vshard/util.lua#L93
> Holy shit, that is a good find. I fixed it in a new commit on top of the
> branch. For all the background fibers at once.
>
>> Also it's possible to wakeup discovery and rebalanser fibers. Probably we should give a way to
>>
>> wakeup master_search_fiber.
> I didn't want people to rely on that. Because when the subscriptions
> are implemented, there won't be a fiber. But I could make it NOP later
> probably.
>
> ====================
> diff --git a/test/router/master_discovery.result b/test/router/master_discovery.result
> index 2fca1e4..6dcdf45 100644
> --- a/test/router/master_discovery.result
> +++ b/test/router/master_discovery.result
> @@ -87,7 +87,7 @@ end
>   function check_all_masters_found()                                              \
>       for _, rs in pairs(vshard.router.static.replicasets) do                     \
>           if not rs.master then                                                   \
> -            vshard.router.static.master_search_fiber:wakeup()                   \
> +            vshard.router.master_search_wakeup()                                \
>               return false                                                        \
>           end                                                                     \
>       end                                                                         \
> @@ -101,7 +101,7 @@ function check_master_for_replicaset(rs_id, master_name)
>       local master_uuid = util.name_to_uuid[master_name]                          \
>       local master = vshard.router.static.replicasets[rs_uuid].master             \
>       if not master or master.uuid ~= master_uuid then                            \
> -        vshard.router.static.master_search_fiber:wakeup()                       \
> +        vshard.router.master_search_wakeup()                                    \
>           return false                                                            \
>       end                                                                         \
>       return true                                                                 \
> @@ -124,7 +124,7 @@ master_search_helper_f = nil
>    | ...
>   function aggressive_master_search_f()                                           \
>       while true do                                                               \
> -        vshard.router.static.master_search_fiber:wakeup()                       \
> +        vshard.router.master_search_wakeup()                                    \
>           fiber.sleep(0.001)                                                      \
>       end                                                                         \
>   end
> diff --git a/test/router/master_discovery.test.lua b/test/router/master_discovery.test.lua
> index e087f58..653725a 100644
> --- a/test/router/master_discovery.test.lua
> +++ b/test/router/master_discovery.test.lua
> @@ -49,7 +49,7 @@ end
>   function check_all_masters_found()                                              \
>       for _, rs in pairs(vshard.router.static.replicasets) do                     \
>           if not rs.master then                                                   \
> -            vshard.router.static.master_search_fiber:wakeup()                   \
> +            vshard.router.master_search_wakeup()                                \
>               return false                                                        \
>           end                                                                     \
>       end                                                                         \
> @@ -61,7 +61,7 @@ function check_master_for_replicaset(rs_id, master_name)
>       local master_uuid = util.name_to_uuid[master_name]                          \
>       local master = vshard.router.static.replicasets[rs_uuid].master             \
>       if not master or master.uuid ~= master_uuid then                            \
> -        vshard.router.static.master_search_fiber:wakeup()                       \
> +        vshard.router.master_search_wakeup()                                    \
>           return false                                                            \
>       end                                                                         \
>       return true                                                                 \
> @@ -78,7 +78,7 @@ end
>   master_search_helper_f = nil
>   function aggressive_master_search_f()                                           \
>       while true do                                                               \
> -        vshard.router.static.master_search_fiber:wakeup()                       \
> +        vshard.router.master_search_wakeup()                                    \
>           fiber.sleep(0.001)                                                      \
>       end                                                                         \
>   end
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 9407ccd..baaeee4 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -1119,6 +1119,13 @@ local function master_search_set(router)
>       end
>   end
>   
> +local function master_search_wakeup(router)
> +    local f = router.master_search_fiber
> +    if f then
> +        f:wakeup()
> +    end
> +end
> +
>   --------------------------------------------------------------------------------
>   -- Configuration
>   --------------------------------------------------------------------------------
> @@ -1545,6 +1552,7 @@ local router_mt = {
>           bootstrap = cluster_bootstrap;
>           bucket_discovery = bucket_discovery;
>           discovery_wakeup = discovery_wakeup;
> +        master_search_wakeup = master_search_wakeup,
>           discovery_set = discovery_set,
>           _route_map_clear = route_map_clear,
>           _bucket_reset = bucket_reset,
> ====================
>
> To the commit message I appended the following text:
>
>      Master discovery works in its own fiber on the router, which is
>      activated only if at least one replicaset is configured to look
>      for the master. It wakes up with a certain period. But it is
>      possible to wake it up on demand using
>      `vshard.router.master_search_wakeup()` function. It does not do
>      anything if master search is not configured for any replicaset.

Thanks for a fix.


  reply	other threads:[~2021-07-05  9:24 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-01 22:09 [Tarantool-patches] [PATCH vshard 0/6] Master discovery Vladislav Shpilevoy via Tarantool-patches
2021-07-01 22:09 ` [Tarantool-patches] [PATCH vshard 1/6] replicaset: introduce netbox_wait_connected() Vladislav Shpilevoy via Tarantool-patches
2021-07-02 11:46   ` Oleg Babin via Tarantool-patches
2021-07-01 22:09 ` [Tarantool-patches] [PATCH vshard 2/6] test: sort some table prints Vladislav Shpilevoy via Tarantool-patches
2021-07-02 11:46   ` Oleg Babin via Tarantool-patches
2021-07-01 22:09 ` [Tarantool-patches] [PATCH vshard 3/6] storage: introduce vshard.storage._call('info') Vladislav Shpilevoy via Tarantool-patches
2021-07-02 11:46   ` Oleg Babin via Tarantool-patches
2021-07-01 22:09 ` [Tarantool-patches] [PATCH vshard 4/6] config: introduce master 'auto' replicaset option Vladislav Shpilevoy via Tarantool-patches
2021-07-02 11:47   ` Oleg Babin via Tarantool-patches
2021-07-02 21:32     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  9:23       ` Oleg Babin via Tarantool-patches
2021-07-01 22:09 ` [Tarantool-patches] [PATCH vshard 5/6] router: introduce automatic master discovery Vladislav Shpilevoy via Tarantool-patches
2021-07-02 11:48   ` Oleg Babin via Tarantool-patches
2021-07-02 21:35     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  9:24       ` Oleg Babin via Tarantool-patches [this message]
2021-07-05 20:53         ` Vladislav Shpilevoy via Tarantool-patches
2021-07-06  8:54           ` Oleg Babin via Tarantool-patches
2021-07-06 21:19             ` Vladislav Shpilevoy via Tarantool-patches
2021-07-01 22:09 ` [Tarantool-patches] [PATCH vshard 6/6] router: update master using a hint from storage Vladislav Shpilevoy via Tarantool-patches
2021-07-02 11:49   ` Oleg Babin via Tarantool-patches
2021-07-02 21:36     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  9:24       ` Oleg Babin via Tarantool-patches
2021-07-05 20:53         ` Vladislav Shpilevoy via Tarantool-patches
2021-07-06  8:55           ` Oleg Babin via Tarantool-patches
2021-07-02 21:36 ` [Tarantool-patches] [PATCH vshard 7/6] util: truncate too long fiber name Vladislav Shpilevoy via Tarantool-patches
2021-07-05  9:23   ` Oleg Babin via Tarantool-patches
2021-08-03 21:55 ` [Tarantool-patches] [PATCH vshard 0/6] Master discovery Vladislav Shpilevoy via Tarantool-patches

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=a78915a3-43b0-bf97-0269-ae4c5fa9016f@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=yaroslav.dynnikov@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH vshard 5/6] router: introduce automatic master discovery' \
    /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