From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E3208469710 for ; Wed, 25 Nov 2020 15:10:49 +0300 (MSK) References: <20201124152405.1174898-1-gorcunov@gmail.com> <20201124152405.1174898-3-gorcunov@gmail.com> <20201125115538.GM875895@grain> From: Serge Petrenko Message-ID: Date: Wed, 25 Nov 2020 15:10:48 +0300 MIME-Version: 1.0 In-Reply-To: <20201125115538.GM875895@grain> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: Cyrill Gorcunov Cc: tml , Vladislav Shpilevoy 25.11.2020 14:55, Cyrill Gorcunov пишет: > 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)); >> >> I'd add a warning for value >= VCLOCK_MAX too. >> >> And since you're checking for value explicitly anyway, IMO it'd be easier to >> follow the code this way (up to you): >> >> if (value < 0) { >>     say_warn(...); >>     value = 1; >> } else if (value >= VCLOCK_MAX) { >>     say_warn(...); >>     value = VCLOCK_MAX - 1; >> } >> return value; > Sure, I don't mind, thanks! > ... >> I'd leave only this part of the comment (or any its variation you like): >> >> Note that at the moment of bootstrap this value is zero. >> We rely on evaluator returning a correct result (quorum = 1) in this case. >> >> 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. No problem. > >>> +/** >>> + * Renew replication_synchro_quorum value if defined >>> + * as a formula and we need to recalculate it. >>> + */ >>> +void >>> +box_renew_replication_synchro_quorum(void) >> 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. I see. I'm not sure we need to implement this. IMO it looks ok now. Let's wait for Vladislav's opinion. > >>> + int quorum = eval_replication_synchro_quorum(value); >>> + if (quorum < 0) >>> + panic("failed to eval replication_synchro_quorum"); >> >> I propose to use the same check we had in >> box_check_replication_synchro_quorum(): >> >> quorum <= 0 || quorum >= VCLOCK_MAX >> >> For now the values are always in correct range (or < 0), but who knows >> how quorum eval may change in future? > Sure, thanks! -- Serge Petrenko