Re: [Tarantool-patches] [PATCH v2 02/10] box: introduce summary RO flag

[Serge Petrenko <sergepetrenko@tarantool.org>]

04.09.2020 01:51, Vladislav Shpilevoy пишет:
> An instance is writable if box.cfg.read_only is false, and it is
> not orphan. Update of the final read-only state of the instance
> needs to fire read-only update triggers, and notify the engines.
> These 2 flags were easy and cheap to check on each operation, and
> the triggers were easy to use since both flags are stored and
> updated inside box.cc.
>
> That is going to change when Raft is introduced. Raft will add 2
> more checks:
>
>    - A flag if Raft is enabled on the node. If it is not, then Raft
>      state won't affect whether the instance is writable;
>
>    - When Raft is enabled, it will allow writes on a leader only.
>
> It means a check for being read-only would look like this:
>
>      is_ro || is_orphan || (raft_is_enabled() && !raft_is_leader())
>
> This is significantly slower. Besides, Raft somehow needs to
> access the read-only triggers and engine API - this looks wrong.
>
> The patch introduces a new flag is_ro_summary. The flag
> incorporates all the read-only conditions into one flag. When some
> subsystem may change read-only state of the instance, it needs to
> call box_update_ro_summary(), and the function takes care of
> updating the summary flag, running the triggers, and notifying the
> engines.
>
> Raft will use this function when its state or config will change.
>
> Needed for #1146

Thanks for the patch! LGTM.


> ---
>   src/box/box.cc | 44 +++++++++++++++++++++++++++-----------------
>   src/box/box.h  |  6 ++++++
>   2 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index faffd5769..0813603c0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -129,6 +129,14 @@ static bool is_local_recovery = false;
>    */
>   static bool is_orphan;
>   
> +/**
> + * Summary flag incorporating all the instance attributes,
> + * affecting ability to write. Currently these are:
> + * - is_ro;
> + * - is_orphan;
> + */
> +static bool is_ro_summary = true;
> +
>   /**
>    * The pool of fibers in the transaction processor thread
>    * working on incoming messages from net, wal and other
> @@ -144,11 +152,24 @@ static struct fiber_pool tx_fiber_pool;
>    */
>   static struct cbus_endpoint tx_prio_endpoint;
>   
> +void
> +box_update_ro_summary(void)
> +{
> +	bool old_is_ro_summary = is_ro_summary;
> +	is_ro_summary = is_ro || is_orphan;
> +	/* In 99% nothing changes. Filter this out first. */
> +	if (is_ro_summary == old_is_ro_summary)
> +		return;
> +
> +	if (is_ro_summary)
> +		engine_switch_to_ro();
> +	fiber_cond_broadcast(&ro_cond);
> +}
> +
>   static int
>   box_check_writable(void)
>   {
> -	/* box is only writable if box.cfg.read_only == false and */
> -	if (is_ro || is_orphan) {
> +	if (is_ro_summary) {
>   		diag_set(ClientError, ER_READONLY);
>   		diag_log();
>   		return -1;
> @@ -253,20 +274,14 @@ box_check_ro(void);
>   void
>   box_set_ro(void)
>   {
> -	bool ro = box_check_ro();
> -	if (ro == is_ro)
> -		return; /* nothing to do */
> -	if (ro)
> -		engine_switch_to_ro();
> -
> -	is_ro = ro;
> -	fiber_cond_broadcast(&ro_cond);
> +	is_ro = box_check_ro();
> +	box_update_ro_summary();
>   }
>   
>   bool
>   box_is_ro(void)
>   {
> -	return is_ro || is_orphan;
> +	return is_ro_summary;
>   }
>   
>   bool
> @@ -293,13 +308,8 @@ box_wait_ro(bool ro, double timeout)
>   void
>   box_do_set_orphan(bool orphan)
>   {
> -	if (is_orphan == orphan)
> -		return; /* nothing to do */
> -	if (orphan)
> -		engine_switch_to_ro();
> -
>   	is_orphan = orphan;
> -	fiber_cond_broadcast(&ro_cond);
> +	box_update_ro_summary();
>   }
>   
>   void
> diff --git a/src/box/box.h b/src/box/box.h
> index f9bd8b98d..5988264a5 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -137,6 +137,12 @@ box_set_orphan(bool orphan);
>   void
>   box_do_set_orphan(bool orphan);
>   
> +/**
> + * Update the final RO flag based on the instance flags and state.
> + */
> +void
> +box_update_ro_summary(void);
> +
>   /**
>    * Iterate over all spaces and save them to the
>    * snapshot file.

-- 
Serge Petrenko