From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id DDEAE445320
 for <tarantool-patches@dev.tarantool.org>;
 Thu,  6 Aug 2020 01:15:48 +0300 (MSK)
References: <0963ccd10f91b249116916d5c1c91f5eef11e438.1596575031.git.v.shpilevoy@tarantool.org>
 <CAK0MaD353JSXrA1VgyJg8ix0w+2EoU2DLVfZTa8eWDcdzCCj4A@mail.gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <326078fd-79f7-7052-37e1-cbbd118e114c@tarantool.org>
Date: Thu, 6 Aug 2020 00:15:47 +0200
MIME-Version: 1.0
In-Reply-To: <CAK0MaD353JSXrA1VgyJg8ix0w+2EoU2DLVfZTa8eWDcdzCCj4A@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Subject: Re: [Tarantool-patches] [PATCH vshard 1/1] storage: allow replica
	to boot before master
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Yaroslav Dynnikov <yaroslav.dynnikov@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>

Hi! Thanks for the review!

>     diff --git a/test/lua_libs/storage_template.lua b/test/lua_libs/storage_template.lua
>     index 29a753d..84e4180 100644
>     --- a/test/lua_libs/storage_template.lua
>     +++ b/test/lua_libs/storage_template.lua
>     @@ -10,12 +12,63 @@ log = require('log')
>      if not cfg.shard_index then
>          cfg.shard_index = 'bucket_id'
>      end
>     +instance_uuid = util.name_to_uuid[NAME]
>     +
>     +--
>     +-- Bootstrap the instance exactly like vshard does. But don't
>     +-- initialize any vshard-specific code.
>     +--
>     +local function boot_like_vshard()
>     +    assert(type(box.cfg) == 'function')
>     +    for rs_uuid, rs in pairs(cfg.sharding) do
>     +        for replica_uuid, replica in pairs(rs.replicas) do
>     +            if replica_uuid == instance_uuid then
>     +                local box_cfg = {replication = {}}
>     +                box_cfg.instance_uuid = replica_uuid
>     +                box_cfg.replicaset_uuid = rs_uuid
>     +                box_cfg.listen = replica.uri
>     +                box_cfg.read_only = not replica.master
>     +                box_cfg.replication_connect_quorum = 0
>     +                box_cfg.replication_timeout = 0.1
>     +                for _, replica in pairs(rs.replicas) do
>     +                    table.insert(box_cfg.replication, replica.uri)
>     +                end
>     +                box.cfg(box_cfg)
>     +                if not replica.master then
>     +                    return
>     +                end
>     +                local uri = luri.parse(replica.uri)
>     +                box.schema.user.create(uri.login, {
>     +                    password = uri.password, if_not_exists = true,
>     +                })
>     +                box.schema.user.grant(uri.login, 'super')
>     +                return
>     +            end
>     +        end
>     +    end
>     +    assert(false)
> 
> 
> Should there be some meaningful message?

Nope. This is just a test, so if it will fail, I will fix it.
Don't want to invent anything here. The code is unreachable,
and assert(false) is telling that fine.

If you mean the message which would help to locate the error -
not needed as well. The assertion fail message will contain file
name and line number.

>     +end
>     +
>     +local omit_cfg = false
>     +local i = 1
>     +while arg[i] ~= nil do
>     +    local key = arg[i]
>     +    i = i + 1
>     +    if key == 'boot_before_cfg' then
>     +        boot_like_vshard()
>     +        omit_cfg = true
>     +    end
>     +end
>     diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_evolution.lua
>     index 5d09b11..f38af74 100644
>     --- a/vshard/storage/reload_evolution.lua
>     +++ b/vshard/storage/reload_evolution.lua
>     @@ -17,6 +17,14 @@ migrations[#migrations + 1] = function(M)
>          -- Code to update Lua objects.
>      end
> 
>     +migrations[#migrations + 1] = function(M)
>     +    local bucket = box.space._bucket
>     +    if bucket then
>     +        assert(M.bucket_on_replace == nil)
>     +        M.bucket_on_replace = bucket:on_replace()[1]
>     +    end
>     +end
>     +
>      --
>      -- Perform an update based on a version stored in `M` (internals).
>      -- @param M Old module internals which should be updated.
>     @@ -33,7 +41,7 @@ local function upgrade(M)
>              log.error(err_msg)
>              error(err_msg)
>          end
>     -    for i = start_version, #migrations  do
>     +    for i = start_version + 1, #migrations do
> 
> 
> Is it already tested somewhere else (migrations I mean)? This change looks like a fix for some other problem.

Good you noticed this. I thought this fix can't be separated, because
reload evolution didn't have any meaningful migrations, and that
couldn't be tested. But after your question I had second thoughts and
it appears the bug does have a "test" already. I extracted it into a
separate commit. Although the test is reverted by the second commit
anyway.

See v2 patchset in a new thread.