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 04D1B6C1AE; Fri, 21 May 2021 22:26:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 04D1B6C1AE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1621625180; bh=/tenKjBvyPer/E9jKG6lntVZ9GH7h0VYSw34MOkaKQg=; 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=zeDd5xDf6t5lr9h95050fo46MMlOZjAaD1O1Jlbxf2jdBcUNnA78kQCgBDUsj9AAy rDOO6gqWYnf7JI1PmvRudkmkdjVgO4KEvxP2knRiq94R04Qvleq3gCSFvndRXI9XCs aeZrBmB0wBO558+8M9BQaBdsOE2z8BWnGN18kRQ8= 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 442E66C1AE for ; Fri, 21 May 2021 22:26:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 442E66C1AE Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lkAmo-0002oi-9G; Fri, 21 May 2021 22:26:18 +0300 To: Oleg Babin , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org References: <04d7d05f09a5ee7ed52b27c480e81232c406e415.1620903962.git.v.shpilevoy@tarantool.org> <00948536-25b1-dcf2-abc7-6e22d9a683e2@tarantool.org> Message-ID: <6df8d63b-9cf3-b859-50a8-931c55c83cd2@tarantool.org> Date: Fri, 21 May 2021 21:26:17 +0200 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: <00948536-25b1-dcf2-abc7-6e22d9a683e2@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91B019B01C53E51AF113D6D562FBA72891FA115C4A817A10000894C459B0CD1B99987701C2FF2D4B5C98E8A7B273B68BA66ABEBE91DA44D54039F47AC1EBC7B57 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70312E9A300D47E3BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063752AC809489EC5B9C8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2098ED7E60FFF8092ECF2DDDEB6D9EEE198738A72DB41484ED2E47CDBA5A96583C09775C1D3CA48CFCF36E64A7E3F8E58117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7A6779F98BF527B7A9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16B5E1C53F199C2BB95B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A59BA72FA75ED03585B3A0755B95E20F9CC04BBF857A093146D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D8FD11F74BDD6E8D1534C44A0ABE3383014CB22D722A0C323FA4695A46808A99622EEB7E7AB02E701D7E09C32AA3244CC0CD03C3D501F60481FEC972FE003D8830363D8B7DA7DD44FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3CIvDNz8QqD45AyE7n44Ng== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638224B4EB3FAA3E0D633162033717A9014A43841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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. >> +    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.