<div dir="ltr"><div dir="ltr"><div>Hi!</div><div><br></div><div>I've got several questions about this patch, find them below.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 13 May 2021 at 14:07, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" target="_blank">v.shpilevoy@tarantool.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">vshard.storage.buckets_count() uses a cached count value when no<br>
changes in _bucket space. The changes are detected using an<br>
on_replace trigger on _bucket space. But it wasn't installed on<br>
the replicas.<br>
<br>
As a result, on replica vshard.storage.buckets_count() didn't<br>
change at all after being called once.<br>
<br>
The patch makes replicas install the trigger on _bucket and update<br>
the bucket generation + drop the caches properly.<br>
<br>
The tricky part is that _bucket might not exist on a replica if it<br>
was vshard-configured earlier than the master. But for that<br>
occasion the trigger installation on _bucket is delayed until<br>
vshard schema is replicated from the master.<br>
<br>
An alternative was to introduce a special version of<br>
buckets_count() on replicas which does not use the cache at all.<br>
But the trigger is going to be useful in scope of #173 where it<br>
should drop bucket refs on the replica.<br>
<br>
Closes #276<br>
Needed for #173<br>
---<br>
test/misc/reconfigure.result | 21 ++++---<br>
test/misc/reconfigure.test.lua | 13 +++--<br>
test/router/boot_replica_first.result | 9 ++-<br>
test/router/boot_replica_first.test.lua | 7 ++-<br>
test/storage/storage.result | 44 ++++++++++++++<br>
test/storage/storage.test.lua | 19 ++++++<br>
vshard/storage/init.lua | 78 ++++++++++++++++++++++---<br>
7 files changed, 164 insertions(+), 27 deletions(-)<br>
<br>
diff --git a/test/misc/reconfigure.result b/test/misc/reconfigure.result<br>
index 3b34841..a03ab5a 100644<br>
--- a/test/misc/reconfigure.result<br>
+++ b/test/misc/reconfigure.result<br>
@@ -271,9 +271,9 @@ box.cfg.read_only<br>
---<br>
- false<br>
...<br>
-#box.space._bucket:on_replace()<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
---<br>
-- 1<br>
+- true<br>
...<br></blockquote><div><br></div><div>Why is this change necessary? Seems now you miss an actual value in case the test fails.</div><div>This question applies to many cases below.</div><div><br></div><div>No matter what testing framework is used, I think that<br></div><div><br></div><div>assert_equals(value, 1) -- assertion failed: expected 1, got 2</div><div><br></div><div>is better (more helpful) than eager<br></div><div><br></div><div>assert(value == 1) -- assertion failed!</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
_ = test_run:switch('storage_2_a')<br>
---<br>
@@ -305,10 +305,13 @@ box.cfg.read_only<br>
---<br>
- true<br>
...<br>
--- Should be zero on the slave node. Even though earlier the node was a master.<br>
-#box.space._bucket:on_replace()<br>
+--<br>
+-- gh-276: replica should have triggers. This is important for proper update of<br>
+-- caches and in future for discarding refs in scope of gh-173.<br>
+--<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
---<br>
-- 0<br>
+- true <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
...</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
_ = test_run:switch('storage_2_b')<br>
---<br>
@@ -340,9 +343,9 @@ box.cfg.read_only<br>
---<br>
- false<br>
...<br>
-#box.space._bucket:on_replace()<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
---<br>
-- 1<br>
+- true<br>
...<br>
_ = test_run:switch('storage_3_a')<br>
---<br>
@@ -373,9 +376,9 @@ box.cfg.read_only<br>
---<br>
- false<br>
...<br>
-#box.space._bucket:on_replace()<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
---<br>
-- 1<br>
+- true<br>
...<br>
_ = test_run:switch('router_1')<br>
---<br>
diff --git a/test/misc/reconfigure.test.lua b/test/misc/reconfigure.test.lua<br>
index 348628c..61ea3c0 100644<br>
--- a/test/misc/reconfigure.test.lua<br>
+++ b/test/misc/reconfigure.test.lua<br>
@@ -109,7 +109,7 @@ table.sort(uris)<br>
uris<br>
box.cfg.replication<br>
box.cfg.read_only<br>
-#box.space._bucket:on_replace()<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
<br>
_ = test_run:switch('storage_2_a')<br>
info = <a href="http://vshard.storage.info" rel="noreferrer" target="_blank">vshard.storage.info</a>()<br>
@@ -119,8 +119,11 @@ table.sort(uris)<br>
uris<br>
box.cfg.replication<br>
box.cfg.read_only<br>
--- Should be zero on the slave node. Even though earlier the node was a master.<br>
-#box.space._bucket:on_replace()<br>
+--<br>
+-- gh-276: replica should have triggers. This is important for proper update of<br>
+-- caches and in future for discarding refs in scope of gh-173.<br>
+--<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
<br>
_ = test_run:switch('storage_2_b')<br>
info = <a href="http://vshard.storage.info" rel="noreferrer" target="_blank">vshard.storage.info</a>()<br>
@@ -130,7 +133,7 @@ table.sort(uris)<br>
uris<br>
box.cfg.replication<br>
box.cfg.read_only<br>
-#box.space._bucket:on_replace()<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
<br>
_ = test_run:switch('storage_3_a')<br>
info = <a href="http://vshard.storage.info" rel="noreferrer" target="_blank">vshard.storage.info</a>()<br>
@@ -140,7 +143,7 @@ table.sort(uris)<br>
uris<br>
box.cfg.replication<br>
box.cfg.read_only<br>
-#box.space._bucket:on_replace()<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
<br>
_ = test_run:switch('router_1')<br>
info = <a href="http://vshard.router.info" rel="noreferrer" target="_blank">vshard.router.info</a>()<br>
diff --git a/test/router/boot_replica_first.result b/test/router/boot_replica_first.result<br>
index 1705230..3c5a08c 100644<br>
--- a/test/router/boot_replica_first.result<br>
+++ b/test/router/boot_replica_first.result<br>
@@ -76,10 +76,13 @@ vshard.storage.call(1, 'read', 'echo', {100})<br>
| message: 'Cannot perform action with bucket 1, reason: Not found'<br>
| name: WRONG_BUCKET<br>
| ...<br>
--- Should not have triggers.<br>
-#box.space._bucket:on_replace()<br>
+--<br>
+-- gh-276: should have triggers. This is important for proper update of caches<br>
+-- and in future for discarding refs in scope of gh-173.<br>
+--<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
| ---<br>
- | - 0<br>
+ | - true<br>
| ...<br>
<br>
test_run:switch('router')<br>
diff --git a/test/router/boot_replica_first.test.lua b/test/router/boot_replica_first.test.lua<br>
index 7b1b3fd..f973f2b 100644<br>
--- a/test/router/boot_replica_first.test.lua<br>
+++ b/test/router/boot_replica_first.test.lua<br>
@@ -29,8 +29,11 @@ test_run:switch('box_1_b')<br>
test_run:wait_lsn('box_1_b', 'box_1_a')<br>
-- Fails, but gracefully. Because the bucket is not found here.<br>
vshard.storage.call(1, 'read', 'echo', {100})<br>
--- Should not have triggers.<br>
-#box.space._bucket:on_replace()<br>
+--<br>
+-- gh-276: should have triggers. This is important for proper update of caches<br>
+-- and in future for discarding refs in scope of gh-173.<br>
+--<br>
+assert(#box.space._bucket:on_replace() == 1)<br>
<br>
test_run:switch('router')<br>
vshard.router.bootstrap()<br>
diff --git a/test/storage/storage.result b/test/storage/storage.result<br>
index d18b7f8..570d9c6 100644<br>
--- a/test/storage/storage.result<br>
+++ b/test/storage/storage.result<br>
@@ -708,6 +708,24 @@ assert(vshard.storage.buckets_count() == 0)<br>
---<br>
- true<br>
...<br>
+test_run:wait_lsn('storage_1_b', 'storage_1_a')<br>
+---<br>
+...<br>
+_ = test_run:switch('storage_1_b')<br>
+---<br>
+...<br>
+--<br>
+-- gh-276: bucket count cache should be properly updated on the replica nodes.<br>
+-- For that the replicas must also install on_replace trigger on _bucket space<br>
+-- to watch for changes.<br>
+--<br>
+assert(vshard.storage.buckets_count() == 0)<br>
+---<br>
+- true<br>
+...<br>
+_ = test_run:switch('storage_1_a')<br>
+---<br>
+...<br>
vshard.storage.bucket_force_create(1, 5)<br>
---<br>
- true<br>
@@ -716,6 +734,19 @@ assert(vshard.storage.buckets_count() == 5)<br>
---<br>
- true<br>
...<br>
+test_run:wait_lsn('storage_1_b', 'storage_1_a')<br>
+---<br>
+...<br>
+_ = test_run:switch('storage_1_b')<br>
+---<br>
+...<br>
+assert(vshard.storage.buckets_count() == 5)<br>
+---<br>
+- true<br>
+...<br>
+_ = test_run:switch('storage_1_a')<br>
+---<br>
+...<br>
vshard.storage.bucket_force_create(6, 5)<br>
---<br>
- true<br>
@@ -724,9 +755,22 @@ assert(vshard.storage.buckets_count() == 10)<br>
---<br>
- true<br>
...<br>
+test_run:wait_lsn('storage_1_b', 'storage_1_a')<br>
+---<br>
+...<br>
+_ = test_run:switch('storage_1_b')<br>
+---<br>
+...<br>
+assert(vshard.storage.buckets_count() == 10)<br>
+---<br>
+- true<br>
+...<br>
--<br>
-- Bucket_generation_wait() registry function.<br>
--<br>
+_ = test_run:switch('storage_1_a')<br>
+---<br>
+...<br>
lstorage = require('vshard.registry').storage<br>
---<br>
...<br>
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua<br>
index 97558f6..494e2e8 100644<br>
--- a/test/storage/storage.test.lua<br>
+++ b/test/storage/storage.test.lua<br>
@@ -201,14 +201,33 @@ for bid, _ in pairs(buckets) do vshard.storage.bucket_force_drop(bid) end<br>
<br>
_ = test_run:switch('storage_1_a')<br>
assert(vshard.storage.buckets_count() == 0)<br>
+test_run:wait_lsn('storage_1_b', 'storage_1_a')<br>
+_ = test_run:switch('storage_1_b')<br>
+--<br>
+-- gh-276: bucket count cache should be properly updated on the replica nodes.<br>
+-- For that the replicas must also install on_replace trigger on _bucket space<br>
+-- to watch for changes.<br>
+--<br>
+assert(vshard.storage.buckets_count() == 0)<br>
+<br>
+_ = test_run:switch('storage_1_a')<br>
vshard.storage.bucket_force_create(1, 5)<br>
assert(vshard.storage.buckets_count() == 5)<br>
+test_run:wait_lsn('storage_1_b', 'storage_1_a')<br>
+_ = test_run:switch('storage_1_b')<br>
+assert(vshard.storage.buckets_count() == 5)<br>
+<br>
+_ = test_run:switch('storage_1_a')<br>
vshard.storage.bucket_force_create(6, 5)<br>
assert(vshard.storage.buckets_count() == 10)<br>
+test_run:wait_lsn('storage_1_b', 'storage_1_a')<br>
+_ = test_run:switch('storage_1_b')<br>
+assert(vshard.storage.buckets_count() == 10)<br>
<br>
--<br>
-- Bucket_generation_wait() registry function.<br>
--<br>
+_ = test_run:switch('storage_1_a')<br>
lstorage = require('vshard.registry').storage<br>
ok, err = lstorage.bucket_generation_wait(-1)<br>
assert(not ok and err.message)<br>
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua<br>
index c4400f7..b1a20d6 100644<br>
--- a/vshard/storage/init.lua<br>
+++ b/vshard/storage/init.lua<br>
@@ -115,6 +115,14 @@ if not M then<br>
-- replace the old function is to keep its reference.<br>
--<br>
bucket_on_replace = nil,<br>
+ --<br>
+ -- Reference to the function used as on_replace trigger on<br>
+ -- _schema space. Saved explicitly by the same reason as<br>
+ -- _bucket on_replace.<br>
+ -- It is used by replicas to wait for schema bootstrap<br>
+ -- because they might be configured earlier than the<br>
+ -- master.<br>
+ schema_on_replace = nil,<br>
-- Fast alternative to box.space._bucket:count(). But may be nil. Reset<br>
-- on each generation change.<br>
bucket_count_cache = nil,<br>
@@ -398,6 +406,62 @@ local function schema_version_make(ver)<br>
return setmetatable(ver, schema_version_mt)<br>
end<br>
<br>
+local function schema_install_triggers()<br>
+ local _bucket = box.space._bucket<br>
+ if M.bucket_on_replace then<br>
+ local ok, err = pcall(_bucket.on_replace, _bucket, nil,<br>
+ M.bucket_on_replace)<br>
+ if not ok then<br>
+ log.warn('Could not drop old trigger from '..<br>
+ '_bucket: %s', err)<br>
+ end<br>
+ end<br>
+ _bucket:on_replace(bucket_generation_increment)<br>
+ M.bucket_on_replace = bucket_generation_increment<br>
+end<br>
+<br>
+local function schema_install_on_replace(_, new)<br>
+ -- Wait not just for _bucket to appear, but for the entire<br>
+ -- schema. This might be important if the schema will ever<br>
+ -- consist of more than just _bucket.<br>
+ if new[1] ~= 'vshard_version' then<br>
+ return<br>
+ end<br>
+ schema_install_triggers()<br>
+<br>
+ local _schema = box.space._schema<br>
+ local ok, err = pcall(_schema.on_replace, _schema, nil, M.schema_on_replace)<br>
+ if not ok then<br>
+ log.warn('Could not drop trigger from _schema inside of the '..<br>
+ 'trigger: %s', err)<br>
+ end<br>
+ M.schema_on_replace = nil<br>
+ -- Drop the caches which might have been created while the<br>
+ -- schema was being replicated.<br>
+ bucket_generation_increment()<br>
+end<br>
+<br>
+--<br>
+-- Install the triggers later when there is an actual schema to install them on.<br>
+-- On replicas it might happen that they are vshard-configured earlier than the<br>
+-- master and therefore don't have the schema right away.<br>
+--<br>
+local function schema_install_triggers_delayed()<br>
+ <a href="http://log.info" rel="noreferrer" target="_blank">log.info</a>('Could not find _bucket space to install triggers - delayed '..<br>
+ 'until the schema is replicated')<br>
+ assert(not box.space._bucket)<br>
+ local _schema = box.space._schema<br>
+ if M.schema_on_replace then<br>
+ local ok, err = pcall(_schema.on_replace, _schema, nil,<br>
+ M.schema_on_replace)<br>
+ if not ok then<br>
+ log.warn('Could not drop trigger from _schema: %s', err)<br>
+ end<br>
+ end<br>
+ _schema:on_replace(schema_install_on_replace)<br>
+ M.schema_on_replace = schema_install_on_replace<br>
+end<br>
+<br>
-- VShard versioning works in 4 numbers: major, minor, patch, and<br>
-- a last helper number incremented on every schema change, if<br>
-- first 3 numbers stay not changed. That happens when users take<br>
@@ -2612,17 +2676,15 @@ local function storage_cfg(cfg, this_replica_uuid, is_reload)<br>
local uri = luri.parse(this_replica.uri)<br>
schema_upgrade(is_master, uri.login, uri.password)<br>
<br>
- if M.bucket_on_replace then<br>
- box.space._bucket:on_replace(nil, M.bucket_on_replace)<br>
- M.bucket_on_replace = nil<br>
- end<br>
- lref.cfg()<br>
- if is_master then<br>
- box.space._bucket:on_replace(bucket_generation_increment)<br>
- M.bucket_on_replace = bucket_generation_increment<br>
+ if is_master or box.space._bucket then<br>
+ schema_install_triggers()<br></blockquote><div><br></div><div>+1 to Oleg, isn't _bucket check enough?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ else<br>
+ schema_install_triggers_delayed()<br>
end<br>
<br>
+ lref.cfg()<br>
lsched.cfg(vshard_cfg)<br>
+<br>
lreplicaset.rebind_replicasets(new_replicasets, M.replicasets)<br>
lreplicaset.outdate_replicasets(M.replicasets)<br>
M.replicasets = new_replicasets<br>
-- <br>
2.24.3 (Apple Git-128)<br>
<br></blockquote><div><br></div><div>P.S. I've tested it on Cartridge. Basic tests (luatest + WebUI inspection) seem to be OK.<br></div><div><br></div><div><div dir="ltr">Best regards</div><div>Yaroslav Dynnikov</div> </div></div></div>