<HTML><BODY><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Пятница, 27 декабря 2019, 21:42 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY">Thanks for the fixes!<br><br>
I've pushed my review fixes on top of this commit. See it below<br>
and on the branch. If you agree, then squash. Otherwise lets<br>
discuss.<br><br>
> Also please check out the discussion with Kostja Osipov in this thread<br>
> ( [Tarantool-patches] [PATCH 4/5] vclock: ignore 0th component in comparisons.),<br>
> there are some new changes to the commit regarding 0 vclock component comparison.<br><br>
Yeah, I saw. Honestly, I don't understand how is 0 component used anywhere<br>
before your patch. From what I see, it was just unused always. I've left a<br>
small amendment in v2 thread to this commit. See answer to<br>
"[Tarantool-patches] [PATCH v2 4/5] vclock: ignore 0th component in comparisons."<br><br>
>> We should document new iproto codes: IPROTO_FETCH_SNAPSHOT,<br>
>> IPROTO_REGISTER, IPROTO_REPLICA_ANON, like here:<br>
>> <a href="https://www.tarantool.io/en/doc/2.2/dev_guide/internals/box_protocol/" target="_blank">https://www.tarantool.io/en/doc/2.2/dev_guide/internals/box_protocol/</a><br>
> <br>
> Opened a ticket. https://github.com/tarantool/doc/issues/1046<br><br>
I rather meant to add them to the docbot request, but this is<br>
ok as well.<br><br>
>> See 4 comments below.<br>
>><br>
>>> diff --git a/src/box/applier.cc b/src/box/applier.cc<br>
>>> index f4f9d0670..2939a0f61 100644<br>
>>> --- a/src/box/applier.cc<br>
>>> +++ b/src/box/applier.cc<br>
>>> @@ -996,10 +1035,24 @@ applier_f(va_list ap)<br>
>>> if (tt_uuid_is_nil(&REPLICASET_UUID)) {<br>
>>> /*<br>
>>> * Execute JOIN if this is a bootstrap.<br>
>>> + * In case of anonymous replication, don't<br>
>>> + * join but just fetch master's snapshot.<br>
>>> + *<br>
>>> * The join will pause the applier<br>
>>> * until WAL is created.<br>
>>> */<br>
>>> -applier_join(applier);<br>
>>> +if (replication_anon)<br>
>>> +applier_fetch_snapshot(applier);<br>
>>> +else<br>
>>> +applier_join(applier);<br>
>>> +}<br>
>>> +if (applier->version_id >= version_id(1, 7, 0) &&<br>
>>> + !replication_anon && instance_id == REPLICA_ID_NIL) {<br>
>>> +/* anonymity was turned off while we were<br>
>>> + * fetching a snapshot or following master.<br>
>>> + * Register the replica now.<br>
>>> + */<br>
>>> +applier_register(applier);<br>
>>> }<br>
>><br>
>> 1. Why do we check for version here? Wouldn't it be more straight<br>
>> to do something like this?<br>
>><br>
>> ================================================================================<br>
>><br>
>> diff --git a/src/box/applier.cc b/src/box/applier.cc<br>
>> index 3efa2b765..32851506c 100644<br>
>> --- a/src/box/applier.cc<br>
>> +++ b/src/box/applier.cc<br>
>> @@ -1032,6 +1032,7 @@ applier_f(va_list ap)<br>
>> while (!fiber_is_cancelled()) {<br>
>> try {<br>
>> applier_connect(applier);<br>
>> +bool was_anon = replication_anon;<br>
>> if (tt_uuid_is_nil(&REPLICASET_UUID)) {<br>
>> /*<br>
>> * Execute JOIN if this is a bootstrap.<br>
>> @@ -1046,8 +1047,8 @@ applier_f(va_list ap)<br>
>> else<br>
>> applier_join(applier);<br>
>> }<br>
>> -if (applier->version_id >= version_id(1, 7, 0) &&<br>
>> - !replication_anon && instance_id == REPLICA_ID_NIL) {<br>
>> +if (was_anon && !replication_anon &&<br>
>> + instance_id == REPLICA_ID_NIL) {<br>
>> /*<br>
>> * Anonymity was turned off while<br>
>> * we were fetching a snapshot or<br>
>><br>
>> ================================================================================<br>
>><br>
>> Also I don't understand, why is it needed? From what I see in<br>
>> box_set_replication_anon(), we stop existing appliers, when<br>
>> change `replication_anon`. So in case `replication_anon` is<br>
>> changed, the fiber should be canceled by that moment.<br>
>> applier_connect() does fiber_testcancel() via coio after each<br>
>> yield.<br>
>><br>
>> A register could even harm here, because this instance will do<br>
>> register twice - from there, and from a new applier fiber, started<br>
>> in box_set_replication_anon().<br>
> <br>
> True, the version check is pointless and the comment is misleading.<br>
> But we need to call applier_register here. In case of a transition<br>
> from anon to normal, we have already fetched a snapshot, so we<br>
> shouldn't execute join in a new applier, we just execute register and<br>
> proceed to subscribe. Fixed.<br><br>
Thanks for the fix. Although I still don't understand how is this<br>
code reachable (I tried to add assert(false), and it crashed). It<br>
is even covered with a test, what is cool btw.</div></div></div></div></blockquote><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
As I see, when we change anon setting, the existing appliers are<br>
cancelled. Why does this applier_f fiber still works, if any change<br>
in anon and replication cancels all the existing appliers?</div></div></div></div></blockquote><br>On replication_anon reconfiguration old appliers are killed.<br>The new applier is started, it connects, checks for replicaset_uuid, which is<br>not nil, since the previous applier has fetched a snapshot,<br>proceeds to checking whether the instance_uuid is zero.<br>If it is (it is, since we were anonymous), and replication_anon is false<br>(and it is, since we have reconfigured it), it proceeds to register.<br>Then it subscribes.<br><br>I guess the previous comment that we fixed here was misleading.<br>This was how I understood it when I started working on this and<br>tried to reuse the same applier without killing it. So, sorry for the<br>confusion.<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
Apparently, I miss something.<br><br>
>>> diff --git a/src/box/box.cc b/src/box/box.cc<br>
>>> index 56d07e634..a93e2503e 100644<br>
>>> --- a/src/box/box.cc<br>
>>> +++ b/src/box/box.cc<br>
>>> @@ -740,6 +770,64 @@ box_set_replication_skip_conflict(void)<br>
>>> replication_skip_conflict = cfg_geti("replication_skip_conflict");<br>
>>> }<br>
>>><br>
>>> +void<br>
>>> +box_set_replication_anon(void)<br>
>>> +{<br>
>>> +bool anon = box_check_replication_anon();<br>
>>> +if (anon == replication_anon)<br>
>>> +return;<br>
>>> +<br>
>>> +if (!anon) {<br>
>>> +/* Turn anonymous instance into a normal one. */<br>
>>> +replication_anon = anon;<br>
>><br>
>> 2. In case of any exception `replication_anon` will contain a not<br>
>> applied value.<br>
>><br>
>> But there is a worse problem - we stop the appliers, and don't<br>
>> restart them in case of an exception. So box_set_replication_anon()<br>
>> is not atomic, it may leave the instance in a half configured<br>
>> state. box_set_replication() deals with that by creation of a new<br>
>> array of appliers. It connects them, and only then atomically<br>
>> replaces the old appliers.<br>
>><br>
>> Also looks like deanonymisation does not respect quorum. I did<br>
>> this test:<br>
>><br>
>> Master:<br>
>> box.cfg{listen=3301}<br>
>> box.schema.user.grant('guest', 'read,write,execute', 'universe')<br>
>><br>
>><br>
>> Replica:<br>
>> box.cfg{replication_anon=true,<br>
>> read_only=true,<br>
>> replication_connect_quorum=0,<br>
>> replication=3301}<br>
>><br>
>> Master:<br>
>> Killed.<br>
>><br>
>> Replica:<br>
>> box.cfg{replication_anon=false, read_only=false}<br>
>><br>
>> The replica hangs. Even though the quorum is 0.<br>
>><br>
>> I think, we somehow need to reuse box_set_replication() for all<br>
>> of this. Looks like it does all we need except does not send the<br>
>> register requests.<br>
> <br>
> True.<br><br>
Cool, I am happy that it was fixed so easy! Although this test<br>
should probably be added to anon.test.lua.<br><br>
Now I've discovered another possible problem. Are anon replicas<br>
going to be used as a failover master in case the old master<br>
dies? Because the example above shows that it is not possible to<br>
make an anon replica read-write without a master. This is what<br>
I get, even with 0 quorum:<br>
---<br>
- error: Couldn't find an instance to register this replica on.<br>
...<br><br>
Is that intended? Likely yes, but I ask just in case.</div></div></div></div></blockquote>True, this is intended.<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
However the thing below looks like a bug:<br><br>
Master:<br>
box.cfg{listen=3301}<br>
box.schema.user.grant('guest', 'read,write,execute', 'universe')<br>
box.schema.user.grant('guest', 'replication')<br><br>
Replica:<br>
box.cfg{replication_anon=true,<br>
read_only=true,<br>
replication_connect_quorum=0,<br>
replication=3301}<br><br>
box.cfg{replication_anon=false}<br>
---<br>
- error: Couldn't find an instance to register this replica on.<br>
...<br><br>
This is because of quorum 0. When the quorum is 0, box_set_replication_anon()<br>
tries to find a leader immediately after creation of appliers. Note, that the<br>
master is alive. I tried to make a simple fix, but failed. Temporary change<br>
to quorum 1 won't help, btw, because it can connect to a read-only replica<br>
instead of a master.<br><br>
Everything works, when I change quorum to 1, and then try deanon again.<br><br>
Also it works when I do this:<br><br>
@@ -789,7 +789,7 @@ box_set_replication_anon(void)<br>
* them can register and others resend a<br>
* non-anonymous subscribe.<br>
*/<br>
- box_sync_replication(false);<br>
+ box_sync_replication(true);<br>
/*<br>
* Wait until the master has registered this<br>
* instance.<br><br>
But I am not sure that it is legal to do so.</div></div></div></div></blockquote><br>Maybe leave it as is for now? I can't see what the consequences<br>will be. If you want to promote an anon replica to master this<br>probably means that one of the masters is dead. So if you do<br>box_sync_replication(true), your instance will hang on replication_anon<br>configuration for replication timeout or whatever (replication_connect_timeout?).<br>You'll need to remove the dead master from box.cfg.replication first.<br>So this will still require additional configuration mangling.<br><br>If you think it's fine to wait for a timeout on replication_anon reconfiguration,<br>feel free to apply the change above. I'm not against it.<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
>>> +/*<br>
>>> + * Reset all appliers. This will interrupt<br>
>>> + * anonymous follow they're in so that one of<br>
>>> + * them can register and others resend a<br>
>>> + * non-anonymous subscribe.<br>
>>> + */<br>
>>> +replicaset_foreach(replica) {<br>
>>> +struct applier *applier = replica->applier;<br>
>>> +if (applier == NULL)<br>
>>> +continue;<br>
>>> +replica_clear_applier(replica);<br>
>>> +applier_stop(applier);<br>
>>> +replica->applier_sync_state = APPLIER_DISCONNECTED;<br>
>>> +replica_set_applier(replica, applier);<br>
>>> +}<br>
>>> +/* Choose a master to send register request to. */<br>
>>> +struct replica *master = replicaset_leader();<br>
>>> +assert(master != NULL && master->applier != NULL);<br>
>>> +struct applier *master_applier = master->applier;<br>
>>> +<br>
>>> +applier_start(master_applier);<br>
>>> +<br>
>>> +applier_resume_to_state(master_applier, APPLIER_REGISTER, TIMEOUT_INFINITY);<br>
>>> +applier_resume_to_state(master_applier, APPLIER_REGISTERED, TIMEOUT_INFINITY);<br>
>>> +applier_resume_to_state(master_applier, APPLIER_READY, TIMEOUT_INFINITY);<br><br>
Is it possible to always wait for APPLIER_READY and not to<br>
pay attention to the prior states?</div></div></div></div></blockquote>No, applier transitions to ready right after CONNECTED.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
Or at least wait for REGISTERED + READY only? Like this:</div></div></div></div></blockquote><br>This is ok. I just liked the verbosity of waiting for all the states step by step.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
================================================================================<br>
}<br>
struct applier *master_applier = master->applier;<br><br>
- applier_resume_to_state(master_applier, APPLIER_READY,<br>
- TIMEOUT_INFINITY);<br>
- applier_resume_to_state(master_applier, APPLIER_REGISTER,<br>
- TIMEOUT_INFINITY);<br>
applier_resume_to_state(master_applier, APPLIER_REGISTERED,<br>
TIMEOUT_INFINITY);<br>
================================================================================<br><br>
>>> +applier_resume(master_applier);<br>
>>> +/**<br>
>>> + * Restart other appliers to<br>
>>> + * resend non-anonymous subscribe.<br>
>>> + */<br>
>>> +replicaset_foreach(replica) {<br>
>>> +if (replica != master && replica->applier)<br>
>>> +applier_start(replica->applier);<br>
>>> +}<br>
>>> +} else if (!is_box_configured) {<br>
>>> +replication_anon = anon;<br>
>>> +} else {<br>
>>> +/*<br>
>>> + * It is forbidden to turn a normal replica into<br>
>>> + * an anonymous one.<br>
>>> + */<br>
>>> +tnt_raise(ClientError, ER_CFG, "replication_anon",<br>
>>> + "cannot be turned on after bootstrap"<br>
>>> + " has finished");<br>
>>> +}<br>
>>> +<br>
>>> +}<br>
>>> diff --git a/src/box/relay.cc b/src/box/relay.cc<br>
>>> index e849fcf4f..22029751f 100644<br>
>>> --- a/src/box/relay.cc<br>
>>> +++ b/src/box/relay.cc<br>
>>> @@ -691,7 +697,10 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,<br>
>>> relay_start(relay, fd, sync, relay_send_row);<br>
>>> auto relay_guard = make_scoped_guard([=] {<br>
>>> relay_stop(relay);<br>
>>> -replica_on_relay_stop(replica);<br>
>>> +if (replica->anon)<br>
>>> +replica_anon_delete(replica);<br>
>>> +else<br>
>>> +replica_on_relay_stop(replica);<br>
>><br>
>> 3. Why can't replica_on_relay_stop() be used for an anon replica?<br>
> <br>
> replica_on_relay_stop() asserts the replica is registered with gc and<br>
> removes it from consumers. Anonymous replicas aren't registered with gc.<br>
> I can rewrite replica_on_relay_stop() appropriately, but why?<br><br>
The problem is that replica_on_relay_stop() is not really a destructor.<br>
It is an event. What to do on that event is encapsulated inside<br>
replica_on_relay_stop(). When you added branching on anon setting, the<br>
encapsulation was broken.</div></div></div></div></blockquote><br>Ok, I see, thanks.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br>
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>
>> index 9dee75b7d..51d157aa3 100644<br>
>> --- a/src/box/lua/load_cfg.lua<br>
>> +++ b/src/box/lua/load_cfg.lua<br>
>> @@ -279,6 +279,7 @@ local dynamic_cfg_order = {<br>
>> replication_sync_timeout = 150,<br>
>> replication_connect_timeout = 150,<br>
>> replication_connect_quorum = 150,<br>
>> + replication_anon = 175,<br>
>> ================================================================================<br>
>><br>
>> I am not sure about this change.<br>
>><br>
>> The only way how can it be changed now - from true to false.<br>
>> Not back. The only dubious scenario: we change `replication_anon`<br>
>> to false, and change `replication` at the same time.<br>
>><br>
>> If we set `replication_anon` false before new `replication`,<br>
>> we will try to register the old `replication`. It means<br>
>> `replication_anon` update should be done after `replication`.<br>
>> So as the new configuration could connect as anon, and then be<br>
>> upgraded to normal.<br>
>><br>
>> But when we will support anonymization of normal replicas, the<br>
>> config update could look like this:<br>
>><br>
>> `replication_anon` false -> true<br>
>> `replication` old -> new<br>
>><br>
>> And here `replication_anon` should be set before `replication`.<br>
>> So as we would connect anonymously to the new replication set.<br>
>><br>
>> In both cases we do unnecessary actions with the old<br>
>> replication.<br>
>><br>
>> Seems like it can't be properly solved by dynamic_cfg_order<br>
>> table. We need to configure `replication_anon` and `replication`<br>
>> at the same time. So as in box_set_replication() you always know<br>
>> if the new `replication` setting is anon or not.<br>
>><br>
>> Maybe we need to extend dynamic_cfg_order idea, and introduce<br>
>> depending parameters. Or parameter groups. Or something like<br>
>> that. So as we could, for example, group `replication_anon` and<br>
>> `replication`, and pass anon value to box_set_replication() when<br>
>> they are updated both at the same box.cfg.<br>
>><br>
>> Or we need to forbid update of some parameters in one box.cfg.<br>
>><br>
>> Or we could make that order of parameters depends on their<br>
>> values. Although it does not solve the problem that when we<br>
>> change both `replication_anon` and `replication`, we do<br>
>> unnecessary actions to the old `replication` in some cases.<br>
> <br>
> Well, I reused the box_set_replication code in replication_anon change, so<br>
> now if replication_anon is changed together with replication, it will trigger<br>
> duplicate box_set_replication action (almost). This is ok, I guess.<br><br>
No, looks like the same problem still exists. The settings<br>
are applied one by one. So when we change anon and replication,<br>
the anon is changed first, and replication is changed second.<br><br>
When anon is changed, it still sees the old replication setting.<br>
Check this code in load_cfg.lua:<br><br>
local function reload_cfg(oldcfg, cfg)<br>
cfg = upgrade_cfg(cfg, translate_cfg)<br>
local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)<br>
local ordered_cfg = {}<br>
-- iterate over original table because prepare_cfg() may store NILs<br>
for key, val in pairs(cfg) do<br>
if dynamic_cfg[key] == nil and oldcfg[key] ~= val then<br>
box.error(box.error.RELOAD_CFG, key);<br>
end<br>
table.insert(ordered_cfg, key)<br>
end<br>
table.sort(ordered_cfg, sort_cfg_cb)<br>
for _, key in pairs(ordered_cfg) do<br>
local val = newcfg[key]<br>
local oldval = oldcfg[key]<br>
if not compare_cfg(val, oldval) then<br>
rawset(oldcfg, key, val)<br>
if not pcall(dynamic_cfg[key]) then<br>
rawset(oldcfg, key, oldval) -- revert the old value<br>
return box.error() -- re-throw<br>
end<br>
if log_cfg_option[key] ~= nil then<br>
val = log_cfg_option[key](val)<br>
end<br>
log.info("set '%s' configuration option to %s", key,<br>
json.encode(val))<br>
end<br>
end<br>
...<br><br>
Here rawset(oldcfg, key, val) is the actual update of what we<br>
see in cfg_get*() functions. So when anon is changed before<br>
replication, it will try to deanon the old replication. And<br>
only then will configure the new replication.<br><br>
In that case lets configure anon after replication in this<br>
patch. It partially solves the problem for now. And I added<br>
a ticket to improve this part.</div></div></div></div></blockquote><br>Thanks for the clarification and fixes, I missed it.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15774721320679164399_BODY"><br><br><a href="https://github.com/tarantool/tarantool/issues/4714" target="_blank">https://github.com/tarantool/tarantool/issues/4714</a><br><br>
Below are my review fixes:<br><br>
================================================================================<br><br>
commit ed559c8f18d5c8606410bfbb76a380a43e2e163f<br>
Author: Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org">v.shpilevoy@tarantool.org</a>><br>
Date: Fri Dec 27 19:56:11 2019 +0300<br><br>
Review fixes 5/5<br><br>
diff --git a/src/box/box.cc b/src/box/box.cc<br>
index f99aeebc6..5a8e4d3b9 100644<br>
--- a/src/box/box.cc<br>
+++ b/src/box/box.cc<br>
@@ -790,7 +790,10 @@ box_set_replication_anon(void)<br>
* non-anonymous subscribe.<br>
*/<br>
box_sync_replication(false);<br>
- /* Choose a master to send register request to. */<br>
+ /*<br>
+ * Wait until the master has registered this<br>
+ * instance.<br>
+ */<br>
struct replica *master = replicaset_leader();<br>
if (master == NULL || master->applier == NULL ||<br>
master->applier->state != APPLIER_CONNECTED) {<br>
@@ -798,10 +801,6 @@ box_set_replication_anon(void)<br>
}<br>
struct applier *master_applier = master->applier;<br><br>
- applier_resume_to_state(master_applier, APPLIER_READY,<br>
- TIMEOUT_INFINITY);<br>
- applier_resume_to_state(master_applier, APPLIER_REGISTER,<br>
- TIMEOUT_INFINITY);<br>
applier_resume_to_state(master_applier, APPLIER_REGISTERED,<br>
TIMEOUT_INFINITY);<br>
applier_resume_to_state(master_applier, APPLIER_READY,<br>
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>
index 51d157aa3..955f50061 100644<br>
--- a/src/box/lua/load_cfg.lua<br>
+++ b/src/box/lua/load_cfg.lua<br>
@@ -279,8 +279,16 @@ local dynamic_cfg_order = {<br>
replication_sync_timeout = 150,<br>
replication_connect_timeout = 150,<br>
replication_connect_quorum = 150,<br>
- replication_anon = 175,<br>
replication = 200,<br>
+ -- Anon is set after `replication` as a temporary workaround<br>
+ -- for the problem, that `replication` and `replication_anon`<br>
+ -- depend on each other. If anon would be configured before<br>
+ -- `replication`, it could lead to a bug, when anon is changed<br>
+ -- from true to false together with `replication`, and it<br>
+ -- would try to deanon the old `replication` before applying<br>
+ -- the new one. This should be fixed when box.cfg is able to<br>
+ -- apply some parameters together and atomically.<br>
+ replication_anon = 250,<br>
}<br><br>
local function sort_cfg_cb(l, r)<br>
diff --git a/src/box/relay.cc b/src/box/relay.cc<br>
index 4258edfcd..7ffc79285 100644<br>
--- a/src/box/relay.cc<br>
+++ b/src/box/relay.cc<br>
@@ -697,10 +697,7 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,<br>
relay_start(relay, fd, sync, relay_send_row);<br>
auto relay_guard = make_scoped_guard([=] {<br>
relay_stop(relay);<br>
- if (replica->anon)<br>
- replica_anon_delete(replica);<br>
- else<br>
- replica_on_relay_stop(replica);<br>
+ replica_on_relay_stop(replica);<br>
});<br><br>
vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);<br>
diff --git a/src/box/replication.cc b/src/box/replication.cc<br>
index 11096cc79..1476c7472 100644<br>
--- a/src/box/replication.cc<br>
+++ b/src/box/replication.cc<br>
@@ -890,14 +890,24 @@ void<br>
replica_on_relay_stop(struct replica *replica)<br>
{<br>
/*<br>
- * If the replica was evicted from the cluster, we don't<br>
- * need to keep WALs for it anymore. Unregister it with<br>
- * the garbage collector then. See also replica_clear_id.<br>
+ * If the replica was evicted from the cluster, or was not<br>
+ * even added there (anon replica), we don't need to keep<br>
+ * WALs for it anymore. Unregister it with the garbage<br>
+ * collector then. See also replica_clear_id.<br>
*/<br>
- assert(replica->gc != NULL);<br>
if (replica->id == REPLICA_ID_NIL) {<br>
- gc_consumer_unregister(replica->gc);<br>
- replica->gc = NULL;<br>
+ if (!replica->anon) {<br>
+ gc_consumer_unregister(replica->gc);<br>
+ replica->gc = NULL;<br>
+ } else {<br>
+ assert(replica->gc == NULL);<br>
+ assert(replica->id == REPLICA_ID_NIL);<br>
+ /*<br>
+ * We do not replicate from anonymous<br>
+ * replicas.<br>
+ */<br>
+ assert(replica->applier == NULL);<br>
+ }<br>
}<br>
if (replica_is_orphan(replica)) {<br>
replica_hash_remove(&replicaset.hash, replica);<br>
@@ -905,17 +915,6 @@ replica_on_relay_stop(struct replica *replica)<br>
}<br>
}<br><br>
-void<br>
-replica_anon_delete(struct replica *replica)<br>
-{<br>
- assert(replica->gc == NULL);<br>
- assert(replica->id == REPLICA_ID_NIL);<br>
- /* We do not replicate from anonymous replicas */<br>
- assert(replica->applier == NULL);<br>
- replica_hash_remove(&replicaset.hash, replica);<br>
- replica_delete(replica);<br>
-}<br>
-<br>
struct replica *<br>
replicaset_first(void)<br>
{<br>
diff --git a/src/box/replication.h b/src/box/replication.h<br>
index 978a09d41..2ef1255b3 100644<br>
--- a/src/box/replication.h<br>
+++ b/src/box/replication.h<br>
@@ -367,9 +367,6 @@ replica_set_applier(struct replica * replica, struct applier * applier);<br>
void<br>
replica_on_relay_stop(struct replica *replica);<br><br>
-void<br>
-replica_anon_delete(struct replica *replica);<br>
-<br>
#if defined(__cplusplus)<br>
} /* extern "C" */<br><br>
diff --git a/test/replication/anon.result b/test/replication/anon.result<br>
index f6a85e7d0..88061569f 100644<br>
--- a/test/replication/anon.result<br>
+++ b/test/replication/anon.result<br>
@@ -324,3 +324,62 @@ box.schema.user.revoke('guest', 'replication')<br>
test_run:cleanup_cluster()<br>
| ---<br>
| ...<br>
+<br>
+--<br>
+-- Check that in case of a master absence an anon replica can't<br>
+-- deanonymize itself, regardless of quorum value.<br>
+--<br>
+test_run:cmd('create server master with script="replication/master1.lua"')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('start server master')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:switch('master')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+box.schema.user.grant('guest', 'replication')<br>
+ | ---<br>
+ | ...<br>
+test_run:cmd('create server replica_anon with rpl_master=master, script="replication/anon1.lua"')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('start server replica_anon')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:switch('replica_anon')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+box.cfg{replication_connect_quorum = 0}<br>
+ | ---<br>
+ | ...<br>
+test_run:cmd('stop server master')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('delete server master')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+box.cfg{replication_anon = false}<br>
+ | ---<br>
+ | - error: Couldn't find an instance to register this replica on.<br>
+ | ...<br>
+test_run:switch('default')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('stop server replica_anon')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('delete server replica_anon')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
diff --git a/test/replication/anon.test.lua b/test/replication/anon.test.lua<br>
index 1877ec8bb..8a8d15c18 100644<br>
--- a/test/replication/anon.test.lua<br>
+++ b/test/replication/anon.test.lua<br>
@@ -116,3 +116,22 @@ box.space.temp:drop()<br>
box.space.loc:drop()<br>
box.schema.user.revoke('guest', 'replication')<br>
test_run:cleanup_cluster()<br>
+<br>
+--<br>
+-- Check that in case of a master absence an anon replica can't<br>
+-- deanonymize itself, regardless of quorum value.<br>
+--<br>
+test_run:cmd('create server master with script="replication/master1.lua"')<br>
+test_run:cmd('start server master')<br>
+test_run:switch('master')<br>
+box.schema.user.grant('guest', 'replication')<br>
+test_run:cmd('create server replica_anon with rpl_master=master, script="replication/anon1.lua"')<br>
+test_run:cmd('start server replica_anon')<br>
+test_run:switch('replica_anon')<br>
+box.cfg{replication_connect_quorum = 0}<br>
+test_run:cmd('stop server master')<br>
+test_run:cmd('delete server master')<br>
+box.cfg{replication_anon = false}<br>
+test_run:switch('default')<br>
+test_run:cmd('stop server replica_anon')<br>
+test_run:cmd('delete server replica_anon')<br>
diff --git a/test/replication/master1.lua b/test/replication/master1.lua<br>
new file mode 120000<br>
index 000000000..1c7debd2f<br>
--- /dev/null<br>
+++ b/test/replication/master1.lua<br>
@@ -0,0 +1 @@<br>
+master.lua<br>
\ No newline at end of file<br></div></div></div></div></blockquote>
<br>LGTM, squashed with a tiny change:<br><br><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">diff --git a/src/box/replication.cc b/src/box/replication.cc</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">index 1476c7472..e7bfa22ab 100644</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">--- a/src/box/replication.cc</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">+++ b/src/box/replication.cc</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">@@ -901,7 +901,6 @@ replica_on_relay_stop(struct replica *replica)</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> replica->gc = NULL;</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> } else {</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> assert(replica->gc == NULL);</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-assert(replica->id == REPLICA_ID_NIL);</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> /*</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> * We do not replicate from anonymous</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 12px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> * replicas.</span><br><br></p>The assertion was inside if (replica->id == REPLICA_ID_NIL) branch.<br>
<br>-- <br>Sergey Petrenko<br></BODY></HTML>