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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the review!

>     diff --git a/test/misc/reconfigure.result b/test/misc/reconfigure.result
>     index 3b34841..a03ab5a 100644
>     --- a/test/misc/reconfigure.result
>     +++ b/test/misc/reconfigure.result
>     @@ -271,9 +271,9 @@ box.cfg.read_only
>      ---
>      - false
>      ...
>     -#box.space._bucket:on_replace()
>     +assert(#box.space._bucket:on_replace() == 1)
>      ---
>     -- 1
>     +- true
>      ...
> 
> 
> Why is this change necessary? Seems now you miss an actual value in case the test fails.
> This question applies to many cases below.

See me response in the first email about the assertions.

> No matter what testing framework is used, I think that
> 
> assert_equals(value, 1) -- assertion failed: expected 1, got 2
> 
> is better (more helpful) than eager
> 
> assert(value == 1) -- assertion failed
Yes, but what you are describing is present in the tap tests
framework. Which currently is hardly usable for complex multi-instance
tests. Hence I use what is available. Decided not to implement my
own testing suite for these checks as it is anyway trivial to
get more info if they start failing.

Until tap would be easier to use for multi-instance tests.

>     @@ -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
>     +        schema_install_triggers()
> 
> 
> +1 to Oleg, isn't _bucket check enough?

I want to emphasize that on the master the bootstrap is finished anyway,
because it performs the bootstrap. On the master no need to check for
_bucket nor wait for the schema anyhow.