From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 19 Jul 2018 10:19:03 +0300 From: Konstantin Osipov Subject: Re: [PATCH v3 07/11] replication: rebootstrap instance on startup if it fell behind Message-ID: <20180719071903.GG11373@chai> References: <64016c63c3727e5df2e4495fe1de52eb8ca5d2eb.1531598427.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <64016c63c3727e5df2e4495fe1de52eb8ca5d2eb.1531598427.git.vdavydov.dev@gmail.com> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [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