From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 E8A28469710 for ; Tue, 17 Nov 2020 15:42:28 +0300 (MSK) References: <637105222f13010a9ed0488ee698b2ba2684b46e.1605570907.git.v.shpilevoy@tarantool.org> From: Serge Petrenko Message-ID: <85c52e01-c876-8d8e-1543-bf272efb1d79@tarantool.org> Date: Tue, 17 Nov 2020 15:42:27 +0300 MIME-Version: 1.0 In-Reply-To: <637105222f13010a9ed0488ee698b2ba2684b46e.1605570907.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 10/12] raft: move box_update_ro_summary to update trigger 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 17.11.2020 03:02, Vladislav Shpilevoy пишет: > box_update_ro_summary() was called from the basic Raft library, > making it depend on box. But it was called every time when Raft > state was changed and broadcasted. It means the same effect can be > achieved by updating RO summary from Raft state update trigger. > > The patch does it, and now Raft code does not depend on box.h. > > Part of #5303 > --- > src/box/raft.c | 2 ++ > src/box/raftlib.c | 8 -------- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/src/box/raft.c b/src/box/raft.c > index f3652bbcb..db1a3f423 100644 > --- a/src/box/raft.c > +++ b/src/box/raft.c > @@ -77,6 +77,8 @@ box_raft_on_update_f(struct trigger *trigger, void *event) > (void)trigger; > struct raft *raft = (struct raft *)event; > assert(raft == box_raft()); > + /* State or enablence could be changed, affecting read-only state. */ > + box_update_ro_summary(); > if (raft->state != RAFT_STATE_LEADER) > return 0; > /* Raft uses synchronous WAL write, corect? So there's a yield in raft_worker_handle_io(). Now there's a period of time when an instance is a follower, but it isn't read-only. When you reconfigure a leader to become voter, everything's fine, since no writing to disk is involved. However, if an existing leader receives a message with term greater, than its own, it'll first persist this term, and thus yield, and proceed to broadcast and switching to ro later. So now it's possible that a follower is writeable for some period of time. Maybe run on_update triggers before the wal write? Even better, run the triggers on the actual state transition. After each  `raft->state = ...`. On the bright side, your patch seems to fix https://github.com/tarantool/tarantool/issues/5440 > diff --git a/src/box/raftlib.c b/src/box/raftlib.c > index 512dbd51f..2e09d5405 100644 > --- a/src/box/raftlib.c > +++ b/src/box/raftlib.c > @@ -33,7 +33,6 @@ > #include "error.h" > #include "fiber.h" > #include "small/region.h" > -#include "box.h" > #include "tt_static.h" > > /** > @@ -603,8 +602,6 @@ raft_sm_become_leader(struct raft *raft) > raft->state = RAFT_STATE_LEADER; > raft->leader = raft->self; > ev_timer_stop(loop(), &raft->timer); > - /* Make read-write (if other subsystems allow that. */ > - box_update_ro_summary(); > /* State is visible and it is changed - broadcast. */ > raft_schedule_broadcast(raft); > } > @@ -655,7 +652,6 @@ raft_sm_schedule_new_term(struct raft *raft, uint64_t new_term) > raft->volatile_vote = 0; > raft->leader = 0; > raft->state = RAFT_STATE_FOLLOWER; > - box_update_ro_summary(); > raft_sm_pause_and_dump(raft); > /* > * State is visible and it is changed - broadcast. Term is also visible, > @@ -686,7 +682,6 @@ raft_sm_schedule_new_election(struct raft *raft) > /* Everyone is a follower until its vote for self is persisted. */ > raft_sm_schedule_new_term(raft, raft->term + 1); > raft_sm_schedule_new_vote(raft, raft->self); > - box_update_ro_summary(); > } > > static void > @@ -771,7 +766,6 @@ raft_sm_start(struct raft *raft) > */ > raft_sm_wait_leader_found(raft); > } > - box_update_ro_summary(); > /* > * Nothing changed. But when raft was stopped, its state wasn't sent to > * replicas. At least this was happening at the moment of this being > @@ -793,7 +787,6 @@ raft_sm_stop(struct raft *raft) > raft->leader = 0; > raft->state = RAFT_STATE_FOLLOWER; > ev_timer_stop(loop(), &raft->timer); > - box_update_ro_summary(); > /* State is visible and changed - broadcast. */ > raft_schedule_broadcast(raft); > } > @@ -879,7 +872,6 @@ raft_cfg_is_candidate(struct raft *raft, bool is_candidate) > raft_schedule_broadcast(raft); > } > } > - box_update_ro_summary(); > } > > void -- Serge Petrenko