From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id AB4EC7030C; Mon, 24 May 2021 09:57:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AB4EC7030C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1621839471; bh=GPjsnZQaOIN8kICu/RTHS2Q5g2sgBcpDjQGIlSt0xuw=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=A/evugTcS9ntwcJXHCqE/tgjyt7a9HpqodXx8LMMfkV832KkZvNPgxAMTPc5vQU3l dseySSjH9K2W04CL0kjLjbjm1wW1o0GbOOfegpcJoSoGGoIUeUO40F9AP43CJfz+8G e6f80INkdO6WZKnK6mJs54F2zetTSLmmoxgURODU= Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 ECD2B7030C for ; Mon, 24 May 2021 09:57:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ECD2B7030C Received: by smtp34.i.mail.ru with esmtpa (envelope-from ) id 1ll4X6-0001jB-Lq; Mon, 24 May 2021 09:57:49 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org References: <04d7d05f09a5ee7ed52b27c480e81232c406e415.1620903962.git.v.shpilevoy@tarantool.org> <00948536-25b1-dcf2-abc7-6e22d9a683e2@tarantool.org> <6df8d63b-9cf3-b859-50a8-931c55c83cd2@tarantool.org> Message-ID: Date: Mon, 24 May 2021 09:57:48 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 MIME-Version: 1.0 In-Reply-To: <6df8d63b-9cf3-b859-50a8-931c55c83cd2@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91B019B01C53E51AF9055931F25A1C7413A6B5BA1B6C38DA100894C459B0CD1B93CE26F98051B32A14AC32F0374CE29F28C4B38FE19A396A87A43E2483F2A4E25 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70CB15FA6C489297DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637175EF79FCE26347D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85E61AB77285D65A80AFD784129247688117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAE9A1BBD95851C5BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEB1593CA6EC85F86DD94E105876FE7799D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3724336BCC0EE1BA82D242C3BD2E3F4C6C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637571EAFDAACF43475EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24209795067102C07E8F7B195E1C978313650886AD966144F437CCAE968C07DD4 X-C1DE0DAB: 0D63561A33F958A58B8A833027B94726F2F706280BBCBB1505FD1BB8954CB259D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A5970F8A4685C8AB8106A36248EE9B323E14C2E01F2F38AF9AE1C3BB9824BDF86D0E3C6D633E79DA1D7E09C32AA3244C5B476A8B4CA0E0FE7A1D3253CA9804FD435BF7150578642FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj+gjVyQcIK6LTbPq8B4cbgA== X-Mailru-Sender: 583F1D7ACE8F49BD07526C4546A62CBFA829B67A40FA70154E3B65C72C11E5E95D0A7B0426F77B8B23E75C7104EB1B885DEE61814008E47C7013064206BFB89F93956FB04BA385BE9437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/2] vshard: fix buckets_count() on replicas X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Oleg Babin via Tarantool-patches Reply-To: Oleg Babin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for your fixes. LGTM. On 21.05.2021 22:26, Vladislav Shpilevoy wrote: > Hi! Thanks for the review! > > On 13.05.2021 22:23, Oleg Babin wrote: >> 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. > Great note! Fixed: > > ==================== > diff --git a/test/router/boot_replica_first.result b/test/router/boot_replica_first.result > index 3c5a08c..d6eaca5 100644 > --- a/test/router/boot_replica_first.result > +++ b/test/router/boot_replica_first.result > @@ -42,6 +42,25 @@ util.check_error(vshard.storage.call, 1, 'read', 'echo', {100}) > | - attempt to index field '_bucket' (a nil value) > | ... > > +-- While waiting for the schema, gracefully handle deletions from _schema. > +ro = box.cfg.read_only > + | --- > + | ... > +box.cfg{read_only = false} > + | --- > + | ... > +box.space._schema:insert({'gh-276'}) > + | --- > + | - ['gh-276'] > + | ... > +box.space._schema:delete({'gh-276'}) > + | --- > + | - ['gh-276'] > + | ... > +box.cfg{read_only = ro} > + | --- > + | ... > + > test_run:switch('default') > | --- > | - true > diff --git a/test/router/boot_replica_first.test.lua b/test/router/boot_replica_first.test.lua > index f973f2b..f1a3c19 100644 > --- a/test/router/boot_replica_first.test.lua > +++ b/test/router/boot_replica_first.test.lua > @@ -19,6 +19,13 @@ vshard.storage.cfg(cfg, instance_uuid) > -- _bucket is not created yet. Will fail. > util.check_error(vshard.storage.call, 1, 'read', 'echo', {100}) > > +-- While waiting for the schema, gracefully handle deletions from _schema. > +ro = box.cfg.read_only > +box.cfg{read_only = false} > +box.space._schema:insert({'gh-276'}) > +box.space._schema:delete({'gh-276'}) > +box.cfg{read_only = ro} > + > test_run:switch('default') > util.map_evals(test_run, {REPLICASET_1}, 'bootstrap_storage(\'memtx\')') > > diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua > index b1a20d6..5285ee0 100644 > --- a/vshard/storage/init.lua > +++ b/vshard/storage/init.lua > @@ -424,7 +424,7 @@ 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 > + if new == nil or new[1] ~= 'vshard_version' then > return > end > schema_install_triggers() > ==================== > >>> +        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. > What do you mean? AFAIS, the third argument is space name, not the space > itself. And I need the space itself to clear the trigger. My bad. Yes, it's a space name not space object. >>> +    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 >>> @@ -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? > 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. Ok.