From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Thu, 6 Aug 2020 01:15:48 +0300 (MSK) References: <0963ccd10f91b249116916d5c1c91f5eef11e438.1596575031.git.v.shpilevoy@tarantool.org> From: Vladislav Shpilevoy 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: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yaroslav Dynnikov Cc: tml 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.