From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 67862431780 for ; Fri, 4 Sep 2020 11:17:32 +0300 (MSK) References: <965674450d3cc47b63c704535247de878986aae6.1599173312.git.v.shpilevoy@tarantool.org> From: Serge Petrenko Message-ID: Date: Fri, 4 Sep 2020 11:17:31 +0300 MIME-Version: 1.0 In-Reply-To: <965674450d3cc47b63c704535247de878986aae6.1599173312.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH v2 02/10] box: introduce summary RO flag List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com 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