[Tarantool-patches] [PATCH v2 5/5] replication: introduce anonymous replica.
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 28 15:15:19 MSK 2019
Thanks for the fixes!
The whole patchset LGTM. With some of my answers below.
> >> 1. Why do we check for version here? Wouldn't it be more straight
> >> to do something like this?
> >>
> >> ================================================================================
> >>
> >> diff --git a/src/box/applier.cc b/src/box/applier.cc
> >> index 3efa2b765..32851506c 100644
> >> --- a/src/box/applier.cc
> >> +++ b/src/box/applier.cc
> >> @@ -1032,6 +1032,7 @@ applier_f(va_list ap)
> >> while (!fiber_is_cancelled()) {
> >> try {
> >> applier_connect(applier);
> >> +bool was_anon = replication_anon;
> >> if (tt_uuid_is_nil(&REPLICASET_UUID)) {
> >> /*
> >> * Execute JOIN if this is a bootstrap.
> >> @@ -1046,8 +1047,8 @@ applier_f(va_list ap)
> >> else
> >> applier_join(applier);
> >> }
> >> -if (applier->version_id >= version_id(1, 7, 0) &&
> >> - !replication_anon && instance_id == REPLICA_ID_NIL) {
> >> +if (was_anon && !replication_anon &&
> >> + instance_id == REPLICA_ID_NIL) {
> >> /*
> >> * Anonymity was turned off while
> >> * we were fetching a snapshot or
> >>
> >> ================================================================================
> >>
> >> Also I don't understand, why is it needed? From what I see in
> >> box_set_replication_anon(), we stop existing appliers, when
> >> change `replication_anon`. So in case `replication_anon` is
> >> changed, the fiber should be canceled by that moment.
> >> applier_connect() does fiber_testcancel() via coio after each
> >> yield.
> >>
> >> A register could even harm here, because this instance will do
> >> register twice - from there, and from a new applier fiber, started
> >> in box_set_replication_anon().
> >
> > True, the version check is pointless and the comment is misleading.
> > But we need to call applier_register here. In case of a transition
> > from anon to normal, we have already fetched a snapshot, so we
> > shouldn't execute join in a new applier, we just execute register and
> > proceed to subscribe. Fixed.
>
> Thanks for the fix. Although I still don't understand how is this
> code reachable (I tried to add assert(false), and it crashed). It
> is even covered with a test, what is cool btw.
>
>
>
>
> As I see, when we change anon setting, the existing appliers are
> cancelled. Why does this applier_f fiber still works, if any change
> in anon and replication cancels all the existing appliers?
>
>
> On replication_anon reconfiguration old appliers are killed.
> The new applier is started, it connects, checks for replicaset_uuid, which is
> not nil, since the previous applier has fetched a snapshot,
> proceeds to checking whether the instance_uuid is zero.
> If it is (it is, since we were anonymous), and replication_anon is false
> (and it is, since we have reconfigured it), it proceeds to register.
> Then it subscribes.
>
> I guess the previous comment that we fixed here was misleading.
> This was how I understood it when I started working on this and
> tried to reuse the same applier without killing it. So, sorry for the
> confusion.
Aha, now I understand. Then looks ok.
> However the thing below looks like a bug:
>
> Master:
> box.cfg{listen=3301}
> box.schema.user.grant('guest', 'read,write,execute', 'universe')
> box.schema.user.grant('guest', 'replication')
>
> Replica:
> box.cfg{replication_anon=true,
> read_only=true,
> replication_connect_quorum=0,
> replication=3301}
>
> box.cfg{replication_anon=false}
> ---
> - error: Couldn't find an instance to register this replica on.
> ...
>
> This is because of quorum 0. When the quorum is 0, box_set_replication_anon()
> tries to find a leader immediately after creation of appliers. Note, that the
> master is alive. I tried to make a simple fix, but failed. Temporary change
> to quorum 1 won't help, btw, because it can connect to a read-only replica
> instead of a master.
>
> Everything works, when I change quorum to 1, and then try deanon again.
>
> Also it works when I do this:
>
> @@ -789,7 +789,7 @@ box_set_replication_anon(void)
> * them can register and others resend a
> * non-anonymous subscribe.
> */
> - box_sync_replication(false);
> + box_sync_replication(true);
> /*
> * Wait until the master has registered this
> * instance.
>
> But I am not sure that it is legal to do so.
>
>
> Maybe leave it as is for now? I can't see what the consequences
> will be. If you want to promote an anon replica to master this
> probably means that one of the masters is dead. So if you do
> box_sync_replication(true), your instance will hang on replication_anon
> configuration for replication timeout or whatever (replication_connect_timeout?).
> You'll need to remove the dead master from box.cfg.replication first.
> So this will still require additional configuration mangling.
>
> If you think it's fine to wait for a timeout on replication_anon reconfiguration,
> feel free to apply the change above. I'm not against it.
I don't know what is more correct to do here. Yes, lets keep it
as is, and look at user feedback.
> >>> +/*
> >>> + * Reset all appliers. This will interrupt
> >>> + * anonymous follow they're in so that one of
> >>> + * them can register and others resend a
> >>> + * non-anonymous subscribe.
> >>> + */
> >>> +replicaset_foreach(replica) {
> >>> +struct applier *applier = replica->applier;
> >>> +if (applier == NULL)
> >>> +continue;
> >>> +replica_clear_applier(replica);
> >>> +applier_stop(applier);
> >>> +replica->applier_sync_state = APPLIER_DISCONNECTED;
> >>> +replica_set_applier(replica, applier);
> >>> +}
> >>> +/* Choose a master to send register request to. */
> >>> +struct replica *master = replicaset_leader();
> >>> +assert(master != NULL && master->applier != NULL);
> >>> +struct applier *master_applier = master->applier;
> >>> +
> >>> +applier_start(master_applier);
> >>> +
> >>> +applier_resume_to_state(master_applier, APPLIER_REGISTER, TIMEOUT_INFINITY);
> >>> +applier_resume_to_state(master_applier, APPLIER_REGISTERED, TIMEOUT_INFINITY);
> >>> +applier_resume_to_state(master_applier, APPLIER_READY, TIMEOUT_INFINITY);
>
> Is it possible to always wait for APPLIER_READY and not to
> pay attention to the prior states?
>
> No, applier transitions to ready right after CONNECTED.
>
>
>
> Or at least wait for REGISTERED + READY only? Like this:
>
>
> This is ok. I just liked the verbosity of waiting for all the states step by step.
You are free to revert this part of my change, I don't mind.
More information about the Tarantool-patches
mailing list