From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Thu, 14 Jun 2018 11:58:37 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v2 08/11] replication: rebootstrap instance on startup if
 it fell behind
Message-ID: <20180614085837.do36dza7aizlm4on@esperanza>
References: <cover.1528478913.git.vdavydov.dev@gmail.com>
 <7864f122209e681a81d4eb59ba3f52188e5051e9.1528478913.git.vdavydov.dev@gmail.com>
 <20180613205527.GE10632@chai>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180613205527.GE10632@chai>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Wed, Jun 13, 2018 at 11:55:27PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/08 20:38]:
> > If a replica fell too much behind its peers in the cluster and xlog
> > files needed for it to get up to speed have been removed, it won't be
> > able to proceed without rebootstrap. This patch makes the recovery
> > procedure detect such cases and initiate rebootstrap procedure if
> > necessary.
> > 
> > Note, rebootstrap is currently only supported by memtx engine. If there
> > are vinyl spaces on the replica, rebootstrap will fail. This is fixed by
> > the following patches.
> 
> A nitpick, but this makes the whole point of factoring out local
> recovery less valid.
> 
> 
> If local_recovery() can fall back to bootstrap_from_master(), then
> the name is misleading.
> 
> Please make sure the control flow and decision making stays in box_cfg().

I rather disagree. Rejoin can be though of as an optional step of the
local recovery process. Although we do overwrite local data, we don't
purge it - it is still in the local directory and can be recovered upon
request. I can, of course, move bootstrap_from_master to box_cfg, but
frankly I think it would make the code look worse.

> 
> >  
> > +bool
> > +replicaset_needs_rejoin(struct replica **master)
> > +{
> > +	replicaset_foreach(replica) {
> > +		if (replica->applier != NULL &&
> > +		    vclock_compare(&replica->applier->gc_vclock,
> > +				   &replicaset.vclock) > 0) {
> > +			*master = replica;
> > +			return true;
> > +		}
> > +	}
> > +	*master = NULL;
> > +	return false;
> > +}
> 
> Intuitively this function should return true only if none of the
> masters can provide it with necessary logs, not *any* of the
> masters.

Well, yes, but if an instance fell behind a master, it won't retry to
subscribe to it, even after it has synced up with a master that has all
xlogs available. I thought about fixing it in the future while leaving
it plain and simple for now - rejoin if at least one master reports the
replica is stale...