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 2/2] vshard: fix buckets_count() on replicas
Date: Thu, 13 May 2021 23:23:26 +0300	[thread overview]
Message-ID: <00948536-25b1-dcf2-abc7-6e22d9a683e2@tarantool.org> (raw)
In-Reply-To: <04d7d05f09a5ee7ed52b27c480e81232c406e415.1620903962.git.v.shpilevoy@tarantool.org>

Hi! Thanks for your patch! Three comments below.

On 13.05.2021 14:07, Vladislav Shpilevoy wrote:
> +local function schema_install_triggers()
> +    local _bucket = box.space._bucket
> +    if M.bucket_on_replace then
> +        local ok, err = pcall(_bucket.on_replace, _bucket, nil,
> +                              M.bucket_on_replace)
> +        if not ok then
> +            log.warn('Could not drop old trigger from '..
> +                     '_bucket: %s', err)
> +        end
> +    end
> +    _bucket:on_replace(bucket_generation_increment)
> +    M.bucket_on_replace = bucket_generation_increment
> +end
> +
> +local function schema_install_on_replace(_, new)
> +    -- Wait not just for _bucket to appear, but for the entire
> +    -- schema. This might be important if the schema will ever
> +    -- consist of more than just _bucket.
> +    if new[1] ~= 'vshard_version' then


Is it possible that someone drops something from _schema space? I mean 
that in

such case new will be nil.

I could provide one such case - when user needs to execute box.once once 
again, (s)he

drops corresponding record from _schema space.

> +        return
> +    end
> +    schema_install_triggers()
> +
> +    local _schema = box.space._schema

nit: you could get it from the third argument of the trigger. But up to you.

> +    local ok, err = pcall(_schema.on_replace, _schema, nil, M.schema_on_replace)
> +    if not ok then
> +        log.warn('Could not drop trigger from _schema inside of the '..
> +                 'trigger: %s', err)
> +    end
> +    M.schema_on_replace = nil
> +    -- Drop the caches which might have been created while the
> +    -- schema was being replicated.
> +    bucket_generation_increment()
> +end
> +
> +--
> +-- Install the triggers later when there is an actual schema to install them on.
> +-- On replicas it might happen that they are vshard-configured earlier than the
> +-- master and therefore don't have the schema right away.
> +--
> +local function schema_install_triggers_delayed()
> +    log.info('Could not find _bucket space to install triggers - delayed '..
> +             'until the schema is replicated')
> +    assert(not box.space._bucket)
> +    local _schema = box.space._schema
> +    if M.schema_on_replace then
> +        local ok, err = pcall(_schema.on_replace, _schema, nil,
> +                              M.schema_on_replace)
> +        if not ok then
> +            log.warn('Could not drop trigger from _schema: %s', err)
> +        end
> +    end
> +    _schema:on_replace(schema_install_on_replace)
> +    M.schema_on_replace = schema_install_on_replace
> +end
> +
>   -- VShard versioning works in 4 numbers: major, minor, patch, and
>   -- a last helper number incremented on every schema change, if
>   -- first 3 numbers stay not changed. That happens when users take
> @@ -2612,17 +2676,15 @@ local function storage_cfg(cfg, this_replica_uuid, is_reload)
>       local uri = luri.parse(this_replica.uri)
>       schema_upgrade(is_master, uri.login, uri.password)
>   
> -    if M.bucket_on_replace then
> -        box.space._bucket:on_replace(nil, M.bucket_on_replace)
> -        M.bucket_on_replace = nil
> -    end
> -    lref.cfg()
> -    if is_master then
> -        box.space._bucket:on_replace(bucket_generation_increment)
> -        M.bucket_on_replace = bucket_generation_increment
> +    if is_master or box.space._bucket then


Is it possible that "is_master" is true and _bucket is nil?

Why simple `if box.space._bucket` is not enough?


  reply	other threads:[~2021-05-13 20:24 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-13 11:07 [Tarantool-patches] [PATCH 0/2] vshard.storage.buckets_count() bug Vladislav Shpilevoy via Tarantool-patches
2021-05-13 11:07 ` [Tarantool-patches] [PATCH 1/2] test: fix test output on latest Tarantool Vladislav Shpilevoy via Tarantool-patches
2021-05-13 20:23   ` Oleg Babin via Tarantool-patches
2021-05-19 21:50   ` Yaroslav Dynnikov via Tarantool-patches
2021-05-21 19:26     ` Vladislav Shpilevoy via Tarantool-patches
2021-05-21 21:54       ` Yaroslav Dynnikov via Tarantool-patches
2021-05-13 11:07 ` [Tarantool-patches] [PATCH 2/2] vshard: fix buckets_count() on replicas Vladislav Shpilevoy via Tarantool-patches
2021-05-13 20:23   ` Oleg Babin via Tarantool-patches [this message]
2021-05-21 19:26     ` Vladislav Shpilevoy via Tarantool-patches
2021-05-24  6:57       ` Oleg Babin via Tarantool-patches
2021-05-19 21:51   ` Yaroslav Dynnikov via Tarantool-patches
2021-05-21 19:30     ` Vladislav Shpilevoy via Tarantool-patches
2021-05-13 20:23 ` [Tarantool-patches] [PATCH 0/2] vshard.storage.buckets_count() bug Oleg Babin via Tarantool-patches
2021-05-25 20:43 ` 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=00948536-25b1-dcf2-abc7-6e22d9a683e2@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 2/2] vshard: fix buckets_count() on replicas' \
    /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