Re: [Tarantool-patches] [PATCH v2 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Serge Petrenko <sergepetrenko@tarantool.org>]

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