Re: [Tarantool-patches] [RFC 2/4] qsync: move synchro quorum update to separate routine

[Serge Petrenko <sergepetrenko@tarantool.org>]

19.11.2020 22:40, Cyrill Gorcunov пишет:
> We will be updating replication_synchro_quorum parameter
> dynamically once we add support for number evaluation
> via symbolic formula, thus the update will be called
> not only from manual configuration changes but from
> cluster updates as well.
>
> Thus to reuse the code lets gather it into a separate
> routine.
>
> Part-of #5446

Hi! Thanks  for  the patch!

Please find my comments below.


> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/box.cc         |  4 +---
>   src/box/replication.cc | 18 ++++++++++++++++++
>   src/box/replication.h  |  3 +++
>   3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1f7dec362..5fcf28cb3 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -924,9 +924,7 @@ box_set_replication_synchro_quorum(void)
>   	int value = box_check_replication_synchro_quorum();
>   	if (value < 0)
>   		return -1;
> -	replication_synchro_quorum = value;
> -	txn_limbo_on_parameters_change(&txn_limbo);
> -	raft_cfg_election_quorum(box_raft());
> +	replication_synchro_quorum_update(value);
>   	return 0;
>   }


IMO it'd be better to name the new function somewhat similar  to
`box_update_replication_synchro_quorum` and leave it in box.cc


This way you avoid adding 2 new dependencies to replication.cc
and leave all the code dealing with reconfiguration at one place.


Besides, replication.cc already depends on box, so it won't be a problem
to call update_replication_synchro_quorum there.


>   
> diff --git a/src/box/replication.cc b/src/box/replication.cc
> index 65512cf0f..c83392f81 100644
> --- a/src/box/replication.cc
> +++ b/src/box/replication.cc
> @@ -40,7 +40,9 @@
>   #include "gc.h"
>   #include "error.h"
>   #include "relay.h"
> +#include "raft.h"
>   #include "sio.h"
> +#include "txn_limbo.h"
>   
>   uint32_t instance_id = REPLICA_ID_NIL;
>   struct tt_uuid INSTANCE_UUID;
> @@ -82,6 +84,22 @@ replicaset_quorum(void)
>   	return MIN(replication_connect_quorum, replicaset.applier.total);
>   }
>   
> +/**
> + * Update the synchro quorum number and propagate the
> + * change to dependent subsystems.
> + */
> +void
> +replication_synchro_quorum_update(int value)
> +{
> +	assert(value > 0 && value < VCLOCK_MAX);
> +
> +	say_info("replication: replication_synchro_quorum = %d", value);


Load_cfg.lua will say something similar, when replication_synchro_quorum is
a number:

```

             log.info("set '%s' configuration option to %s", key,
                 json.encode(val))

```

So this say_info belongs to the trigger you invoke on replica 
register/unregister.


> +
> +	replication_synchro_quorum = value;
> +	txn_limbo_on_parameters_change(&txn_limbo);
> +	raft_cfg_election_quorum(box_raft());
> +}
> +
>   void
>   replication_init(void)
>   {
> diff --git a/src/box/replication.h b/src/box/replication.h
> index e57912848..ced519612 100644
> --- a/src/box/replication.h
> +++ b/src/box/replication.h
> @@ -175,6 +175,9 @@ replication_disconnect_timeout(void)
>   	return replication_timeout * 4;
>   }
>   
> +void
> +replication_synchro_quorum_update(int value);
> +
>   void
>   replication_init(void);
>   

-- 
Serge Petrenko