[Tarantool-patches] [PATCH] replication: remove unnecessary errors on replicating from an anonymous instance
Serge Petrenko
sergepetrenko at tarantool.org
Wed May 6 13:08:48 MSK 2020
28.04.2020 23:55, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
Thanks for the review!
>
> On 20/04/2020 15:11, Serge Petrenko wrote:
>> Since the anonymous replica implementation, it was forbidden to
>> replicate (join/subscribe/register) from anonymous instances.
>> Actually, only joining and register should be banned. Anonymous replica
>> isn't able to register its peer in _cluster anyway.
>> Allow subscribing to anonymous instances and remove all the errors
>> mentioning unsupported replication from anonymous replica. Join and
>> register are covered by ER_READONLY errors, since anonymous replicas
>> must be read-only.
> Seems like some amount of double whitespaces here and in the patch
> (I noticed people with new macbooks have them often, huh).
Thanks, fixed. This keybboard is killing me.
>> Closes #4696
>> ---
>> https://github.com/tarantool/tarantool/tree/sp/gh-4696-anon-replica-follow
>> https://github.com/tarantool/tarantool/issues/4696
>>
>> There's one open question: should we allow 'normal' instances to follow
>> anonymous instances? The current patch implementation allows this.
>> I see one possible use for allowing normal instances to follow anonymous ones:
>> in case such an anonymous instance can be failed over to with its transition to
>> master. Then it would be better if all the cluster members followed it in
>> advance, to instantly start receiving changes from it when it becomes 'normal'
>> and read-write.
>>
>> @ChangeLog:
>> - Allow anonymous replicas to be followed by other ones (gh-4696).
>>
>> diff --git a/test/replication/anon.result b/test/replication/anon.result
>> index cbbeeef09..997112c84 100644
>> --- a/test/replication/anon.result
>> +++ b/test/replication/anon.result
>> @@ -388,3 +323,167 @@ test_run:cmd('delete server replica_anon')
>> | ---
>> | - true
>> | ...
>> +
>> +--
>> +-- gh-4696. Following an anonymous replica.
>> +--
>> +box.schema.user.grant('guest', 'replication')
>> + | ---
>> + | ...
>> +
>> +test_run:cmd([[create server replica_anon1 with rpl_master=default,\
>> + script="replication/anon1.lua"]])
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd([[create server replica_anon2 with rpl_master=replica_anon1,\
>> + script="replication/anon2.lua"]])
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('start server replica_anon1')
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('start server replica_anon2')
>> + | ---
>> + | - true
>> + | ...
>> +_ = box.schema.space.create('test')
>> + | ---
>> + | ...
>> +_ = box.space.test:create_index('pk')
>> + | ---
>> + | ...
>> +box.space.test:insert{1}
>> + | ---
>> + | - [1]
>> + | ...
>> +
>> +-- Check that master's changes are propagated to replica2,
>> +-- following an anonymous replica1.
>> +test_run:cmd('switch replica_anon2')
>> + | ---
>> + | - true
>> + | ...
>> +box.space.test:select{}
>> + | ---
>> + | - - [1]
>> + | ...
> Nit: the test is potentially flaky. The change may not manage
> to propagate to replica_anon2 while you switch to it. A more
> robust option would be to use test_run:wait_cond() to wait
> until the space has this tuple.
Added wait_cond()
>> +
>> +test_run:cmd('switch default')
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('stop server replica_anon2')
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('delete server replica_anon2')
>> + | ---
>> + | - true
>> + | ...
>> +
>> +-- Check that joining to an anonymous replica is prohibited.
>> +test_run:cmd([[create server replica with rpl_master=replica_anon1,\
>> + script="replication/replica.lua"]])
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('start server replica with crash_expected=True')
>> + | ---
>> + | - false
>> + | ...
>> +test_run:cmd('delete server replica')
>> + | ---
>> + | - true
>> + | ...
>> +
>> +-- A normal instance (already joined) can follow an anonymous
>> +-- replica.
>> +test_run:cmd([[create server replica with rpl_master=default,\
>> + script="replication/replica.lua"]])
> Isn't it dangerous? So the master thinks this replica is dead,
> even though it is not, but just connected through an anon proxy?
> In case it is true, and the master has a replication quorum, that
> may turn him into orphan when the normal replicas are connected
> via an anon replica.
>
> This is related to your question whether we should allow to normal
> replicas replicate from anon. I don't have a strong opinion here.
> I just was always thinking that anon replicas are some kind of
> satellite instances which never become a master, or even a read-only
> replica unless everything else is dead.
Ok then, let's prohibit normal replicas to follow anonymous ones.
I've updated the test case and commit message
The diff is below.
diff --git a/src/box/applier.cc b/src/box/applier.cc
index 96068f098..c6deeff1b 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -640,6 +640,8 @@ applier_read_tx(struct applier *applier, struct
stailq *rows)
/*
* A safety net, this can only occur
* if we're fed a strangely broken xlog.
+ * row->replica_id == 0, when reading
+ * heartbeats from an anonymous instance.
*/
tnt_raise(ClientError, ER_UNKNOWN_REPLICA,
int2str(row->replica_id),
diff --git a/src/box/box.cc b/src/box/box.cc
index 7a272900c..f9b7d5bcf 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1813,6 +1819,15 @@ box_process_subscribe(struct ev_io *io, struct
xrow_header *header)
if (tt_uuid_is_equal(&replica_uuid, &INSTANCE_UUID))
tnt_raise(ClientError, ER_CONNECTION_TO_SELF);
+ /*
+ * Do not allow non-anonymous followers for anonymous
+ * instances.
+ */
+ if (replication_anon && !anon) {
+ tnt_raise(ClientError, ER_UNSUPPORTED, "Anonymous replica",
+ "non-anonymous followers.");
+ }
+
/* Check permissions */
access_check_universe_xc(PRIV_R);
diff --git a/test/replication/anon.result b/test/replication/anon.result
index 997112c84..8782212ca 100644
--- a/test/replication/anon.result
+++ b/test/replication/anon.result
@@ -330,6 +330,12 @@ test_run:cmd('delete server replica_anon')
box.schema.user.grant('guest', 'replication')
| ---
| ...
+_ = box.schema.space.create('test')
+ | ---
+ | ...
+_ = box.space.test:create_index('pk')
+ | ---
+ | ...
test_run:cmd([[create server replica_anon1 with rpl_master=default,\
script="replication/anon1.lua"]])
@@ -349,12 +355,7 @@ test_run:cmd('start server replica_anon2')
| ---
| - true
| ...
-_ = box.schema.space.create('test')
- | ---
- | ...
-_ = box.space.test:create_index('pk')
- | ---
- | ...
+
box.space.test:insert{1}
| ---
| - [1]
@@ -366,9 +367,9 @@ test_run:cmd('switch replica_anon2')
| ---
| - true
| ...
-box.space.test:select{}
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end, 10)
| ---
- | - - [1]
+ | - true
| ...
test_run:cmd('switch default')
@@ -399,14 +400,14 @@ test_run:cmd('delete server replica')
| - true
| ...
--- A normal instance (already joined) can follow an anonymous
+-- A normal instance (already joined) can't follow an anonymous
-- replica.
test_run:cmd([[create server replica with rpl_master=default,\
script="replication/replica.lua"]])
| ---
| - true
| ...
-test_run:cmd('start server replica')
+test_run:cmd('start server replica')
| ---
| - true
| ...
@@ -429,36 +430,10 @@ test_run:cmd('set variable repl_source to
"replica_anon1.listen"')
box.cfg{replication=repl_source}
| ---
| ...
-
-test_run:cmd('switch default')
- | ---
- | - true
- | ...
-box.space.test:insert{2}
- | ---
- | - [2]
- | ...
-
-test_run:cmd('switch replica_anon1')
- | ---
- | - true
- | ...
--- Check that the new replica follows an anonymous one.
-test_run:wait_downstream(2, {status='follow'})
+test_run:wait_log('replica', 'ER_UNSUPPORTED: Anonymous replica does
not support non.anonymous followers.', nil, 10)
| ---
- | - true
+ | - 'ER_UNSUPPORTED: Anonymous replica does not support non-anonymous
followers.'
| ...
-
-test_run:cmd('switch replica')
- | ---
- | - true
- | ...
-box.space.test:select{}
- | ---
- | - - [1]
- | - [2]
- | ...
-
test_run:cmd('switch default')
| ---
| - true
diff --git a/test/replication/anon.test.lua b/test/replication/anon.test.lua
index cdbb8c7bb..39393e24a 100644
--- a/test/replication/anon.test.lua
+++ b/test/replication/anon.test.lua
@@ -115,6 +115,8 @@ test_run:cmd('delete server replica_anon')
-- gh-4696. Following an anonymous replica.
--
box.schema.user.grant('guest', 'replication')
+_ = box.schema.space.create('test')
+_ = box.space.test:create_index('pk')
test_run:cmd([[create server replica_anon1 with rpl_master=default,\
script="replication/anon1.lua"]])
@@ -122,14 +124,13 @@ test_run:cmd([[create server replica_anon2 with
rpl_master=replica_anon1,\
script="replication/anon2.lua"]])
test_run:cmd('start server replica_anon1')
test_run:cmd('start server replica_anon2')
-_ = box.schema.space.create('test')
-_ = box.space.test:create_index('pk')
+
box.space.test:insert{1}
-- Check that master's changes are propagated to replica2,
-- following an anonymous replica1.
test_run:cmd('switch replica_anon2')
-box.space.test:select{}
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end, 10)
test_run:cmd('switch default')
test_run:cmd('stop server replica_anon2')
@@ -141,27 +142,17 @@ test_run:cmd([[create server replica with
rpl_master=replica_anon1,\
test_run:cmd('start server replica with crash_expected=True')
test_run:cmd('delete server replica')
--- A normal instance (already joined) can follow an anonymous
+-- A normal instance (already joined) can't follow an anonymous
-- replica.
test_run:cmd([[create server replica with rpl_master=default,\
script="replication/replica.lua"]])
-test_run:cmd('start server replica')
+test_run:cmd('start server replica')
test_run:cmd('switch replica')
test_run:wait_upstream(1, {status='follow'})
box.info.id
test_run:cmd('set variable repl_source to "replica_anon1.listen"')
box.cfg{replication=repl_source}
-
-test_run:cmd('switch default')
-box.space.test:insert{2}
-
-test_run:cmd('switch replica_anon1')
--- Check that the new replica follows an anonymous one.
-test_run:wait_downstream(2, {status='follow'})
-
-test_run:cmd('switch replica')
-box.space.test:select{}
-
+test_run:wait_log('replica', 'ER_UNSUPPORTED: Anonymous replica does
not support non.anonymous followers.', nil, 10)
test_run:cmd('switch default')
-- Cleanup.
--
--
Serge Petrenko
More information about the Tarantool-patches
mailing list