From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E3B6F469719 for ; Sat, 29 Feb 2020 11:54:49 +0300 (MSK) Received: by mail-lj1-f196.google.com with SMTP id 143so6035212ljj.7 for ; Sat, 29 Feb 2020 00:54:49 -0800 (PST) Date: Sat, 29 Feb 2020 11:54:46 +0300 From: Konstantin Osipov Message-ID: <20200229085446.GB8676@atlas> References: <20200228170130.81713-1-sergepetrenko@tarantool.org> <468ada69-df04-0564-4c01-1ccab99b535c@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <468ada69-df04-0564-4c01-1ccab99b535c@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] replication: fix rebootstrap in case the instance is listed in box.cfg.replication List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: kirichenkoga@gmail.com, tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [20/02/29 08:50]: > Thanks for the patch! > > On 28/02/2020 18:01, Serge Petrenko wrote: > > When checking wheter rejoin is needed, replica loops through all the > > instances in box.cfg.replication, which makes it believe that there is a > > master holding files, needed by it, since it accounts itself just like > > all other instances. > > So make replica skip itself when finding an instance which holds files > > needed by it, and determining whether rebootstrap is needed. > > > > We already have a working test for the issue, it missed the issue due to > > replica.lua replication settings. Fix replica.lua to optionally include > > itself in box.cfg.replication, so that the corresponding test works > > correctly. > > > > Closes #4759 > > --- > > https://github.com/tarantool/tarantool/issues/4759 > > https://github.com/tarantool/tarantool/tree/sp/gh-4759-rebootstrap-fix > > > > @ChangeLog > > - fix rebootstrap procedure not working in case replica itself > > is listed in `box.cfg.replication` > > > > src/box/replication.cc | 13 ++++++++++++- > > test/replication/replica.lua | 11 ++++++++++- > > test/replication/replica_rejoin.result | 12 ++++++------ > > test/replication/replica_rejoin.test.lua | 12 ++++++------ > > 4 files changed, 34 insertions(+), 14 deletions(-) > > > > diff --git a/src/box/replication.cc b/src/box/replication.cc > > index e7bfa22ab..01edc0fb2 100644 > > --- a/src/box/replication.cc > > +++ b/src/box/replication.cc > > @@ -768,8 +768,19 @@ replicaset_needs_rejoin(struct replica **master) > > struct replica *leader = NULL; > > replicaset_foreach(replica) { > > struct applier *applier = replica->applier; > > - if (applier == NULL) > > + /* > > + * The function is called right after > > + * box_sync_replication(), which in turn calls > > + * replicaset_connect(), which ensures that > > + * appliers are either stopped (APPLIER OFF) or > > + * connected. > > + * Also ignore self, as self applier might not > > + * have disconnected yet. > > + */ > > + if (applier == NULL || applier->state == APPLIER_OFF || > > + tt_uuid_is_equal(&replica->uuid, &INSTANCE_UUID)) > > continue; > > + assert(applier->state == APPLIER_CONNECTED); > > > > Could you please understand one thing? Below I see this: > > > const struct ballot *ballot = &applier->ballot; > > if (vclock_compare(&ballot->gc_vclock, > > &replicaset.vclock) <= 0) { > > /* > > * There's at least one master that still stores > > * WALs needed by this instance. Proceed to local > > * recovery. > > */ > > return false; > > } > > Question is why do we need rebootstrap if some remote node's > vclock is bigger than ours? It does not mean anything. It doesn't > say whether that remote instance still keeps any xlogs. All it > tells is that the remote instance committed some more data since > our restart. During rebootstrap we don't care about WAL logs too much, we bootstrap off the latest checkpoint, and then feed the logs since the latest checkpoint. Could be added in a comment. -- Konstantin Osipov, Moscow, Russia