From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) (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 82D09469710 for ; Wed, 25 Nov 2020 14:55:42 +0300 (MSK) Received: by mail-lf1-f46.google.com with SMTP id l11so2781968lfg.0 for ; Wed, 25 Nov 2020 03:55:42 -0800 (PST) Date: Wed, 25 Nov 2020 14:55:38 +0300 From: Cyrill Gorcunov Message-ID: <20201125115538.GM875895@grain> References: <20201124152405.1174898-1-gorcunov@gmail.com> <20201124152405.1174898-3-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 2/3] cfg: support symbolic evaluation of replication_synchro_quorum List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tml , Vladislav Shpilevoy On Wed, Nov 25, 2020 at 02:36:18PM +0300, Serge Petrenko wrote: ... > > + if (value < 0) { > > + say_warn("replication_synchro_quorum evaluated " > > + "to the negative value %d, ignore", value); > > + } > > + return MAX(1, MIN(value, VCLOCK_MAX-1)); >=20 >=20 > I'd add a warning for value >=3D VCLOCK_MAX too. >=20 > And since you're checking for value explicitly anyway, IMO it'd be easier= to > follow the code this way (up to you): >=20 > if (value < 0) { > =A0=A0=A0 say_warn(...); > =A0=A0=A0 value =3D 1; > } else if (value >=3D VCLOCK_MAX) { > =A0=A0=A0 say_warn(...); > =A0=A0=A0 value =3D VCLOCK_MAX - 1; > } > return value; Sure, I don't mind, thanks! ... >=20 > I'd leave only this part of the comment (or any its variation you like): >=20 > Note that at the moment of bootstrap this value is zero. > We rely on evaluator returning a correct result (quorum =3D 1) in this ca= se. >=20 > Or maybe simply pass MAX(1, replicaset.registered_count) each time? > With a relevant comment about bootstrap. You know I would prefer all manipulations with data range to be inside the evaluation function (including min/max and etc), This will allow us to modify operations in one place if something get changed in future and we eliminate requirements from the caller side. IOW, the caller may pass whatever it wants but will get valid results all the time (except syntax errors of course). So I think idea of shrinking comment is better. But lets return to this place once I update the code. Won't be hard to change. > > +/** > > + * Renew replication_synchro_quorum value if defined > > + * as a formula and we need to recalculate it. > > + */ > > +void > > +box_renew_replication_synchro_quorum(void) >=20 > What do you think of `box_update_replication_synchro_quorum`? I don't mind with any name, so I'llupdate it to match. You know the most thing which bothers me most is the fact that we're calling box function from a deep code chain of replication engine. replica_set/clear helpers are bound to replication internals and ideally should know nothing about box configuration that's why I thought of some kind of notification hooks or triggers. Say replicaset allocates a trigger on init and allow any code to be notified with stage changes. In our case the stage is replica id set or clear. Thus box could setup a trigger into replicaset and we simply run the trigger. For me this would look a way more natural but I'm not sure, because this will require to introduce "stages" and instead of a single call to box_ we will have a way more bigger patch with unclear picture... Simply dunno. > > + int quorum =3D eval_replication_synchro_quorum(value); > > + if (quorum < 0) > > + panic("failed to eval replication_synchro_quorum"); >=20 >=20 > I propose to use the same check we had in > box_check_replication_synchro_quorum(): >=20 > quorum <=3D 0 || quorum >=3D VCLOCK_MAX >=20 > For now the values are always in correct range (or < 0), but who knows > how quorum eval may change in future? Sure, thanks!