Re: [PATCH v3 07/11] replication: rebootstrap instance on startup if it fell behind

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/07/14 23:50]:

> diff --git a/src/box/box.cc b/src/box/box.cc
> index b629a4d8..baf30fce 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1797,6 +1797,9 @@ bootstrap(const struct tt_uuid *instance_uuid,
>  /**
>   * Recover the instance from the local directory.
>   * Enter hot standby if the directory is locked.
> + * Invoke rebootstrap if the instance fell too much
> + * behind its peers in the replica set and needs
> + * to be rebootstrapped.
>   */
>  static void
>  local_recovery(const struct tt_uuid *instance_uuid,
> @@ -1832,6 +1835,12 @@ local_recovery(const struct tt_uuid *instance_uuid,
>  	if (wal_dir_lock >= 0) {
>  		box_listen();
>  		box_sync_replication(replication_connect_timeout, false);
> +
> +		struct replica *master;
> +		if (replicaset_needs_rejoin(&master)) {
> +			say_info("replica is too old, initiating rejoin");
> +			return bootstrap_from_master(master);

This is say_crit() IMHO.

> +bool
> +replicaset_needs_rejoin(struct replica **master)
> +{
> +	replicaset_foreach(replica) {
> +		/*
> +		 * Rebootstrap this instance from a master if:
> +		 * - the oldest vclock stored on the master is greater
> +		 *   than or incomparable with the instance vclock
> +		 *   (so that the instance can't follow the master) and
> +		 * - the instance is strictly behind the master (so
> +		 *   that we won't lose any data by rebootstrapping
> +		 *   this instance)
> +		 */
> +		struct applier *applier = replica->applier;
> +		if (applier != NULL &&
> +		    vclock_compare(&applier->remote_status.gc_vclock,
> +				   &replicaset.vclock) > 0 &&
> +		    vclock_compare(&replicaset.vclock,
> +				   &applier->remote_status.vclock) < 0) {
> +			*master = replica;
> +			return true;

I'd love to see a bit more clarity in the log about this decision
making process. Imagine this function returns 'false' because
vclocks are incomparable and then replication breaks - it would be
very hard to diagnose why this happened. You could add some
logging to this function, but this would change its contract, since
currently this function has no side effects.

Should it set the diagnostics area in case of error? Log the
error? Return an extra status code? Please feel free to choose the
option you think is best.

Thank you for working on this,

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov