[Tarantool-patches] [PATCH 2/2] vshard: fix buckets_count() on replicas

[Oleg Babin]

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?