From: Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/2] vshard: fix buckets_count() on replicas Date: Mon, 24 May 2021 09:57:48 +0300 [thread overview] Message-ID: <fb1d5b99-9123-3661-1997-5e22b7a711d3@tarantool.org> (raw) In-Reply-To: <6df8d63b-9cf3-b859-50a8-931c55c83cd2@tarantool.org> 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.
next prev parent reply other threads:[~2021-05-24 6:57 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-13 11:07 [Tarantool-patches] [PATCH 0/2] vshard.storage.buckets_count() bug Vladislav Shpilevoy via Tarantool-patches 2021-05-13 11:07 ` [Tarantool-patches] [PATCH 1/2] test: fix test output on latest Tarantool Vladislav Shpilevoy via Tarantool-patches 2021-05-13 20:23 ` Oleg Babin via Tarantool-patches 2021-05-19 21:50 ` Yaroslav Dynnikov via Tarantool-patches 2021-05-21 19:26 ` Vladislav Shpilevoy via Tarantool-patches 2021-05-21 21:54 ` Yaroslav Dynnikov via Tarantool-patches 2021-05-13 11:07 ` [Tarantool-patches] [PATCH 2/2] vshard: fix buckets_count() on replicas Vladislav Shpilevoy via Tarantool-patches 2021-05-13 20:23 ` Oleg Babin via Tarantool-patches 2021-05-21 19:26 ` Vladislav Shpilevoy via Tarantool-patches 2021-05-24 6:57 ` Oleg Babin via Tarantool-patches [this message] 2021-05-19 21:51 ` Yaroslav Dynnikov via Tarantool-patches 2021-05-21 19:30 ` Vladislav Shpilevoy via Tarantool-patches 2021-05-13 20:23 ` [Tarantool-patches] [PATCH 0/2] vshard.storage.buckets_count() bug Oleg Babin via Tarantool-patches 2021-05-25 20:43 ` Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=fb1d5b99-9123-3661-1997-5e22b7a711d3@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=olegrok@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --cc=yaroslav.dynnikov@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] vshard: fix buckets_count() on replicas' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox