Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Oleg Babin <olegrok@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: Fri, 21 May 2021 21:26:17 +0200	[thread overview]
Message-ID: <6df8d63b-9cf3-b859-50a8-931c55c83cd2@tarantool.org> (raw)
In-Reply-To: <00948536-25b1-dcf2-abc7-6e22d9a683e2@tarantool.org>

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.

  reply	other threads:[~2021-05-21 19:26 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 [this message]
2021-05-24  6:57       ` Oleg Babin via Tarantool-patches
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=6df8d63b-9cf3-b859-50a8-931c55c83cd2@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