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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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.

> > +/**
> > + * 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.

> > +	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!