From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 09D67469710 for ; Tue, 17 Nov 2020 18:17:58 +0300 (MSK) From: Serge Petrenko References: <637105222f13010a9ed0488ee698b2ba2684b46e.1605570907.git.v.shpilevoy@tarantool.org> <85c52e01-c876-8d8e-1543-bf272efb1d79@tarantool.org> Message-ID: <06a43da6-3293-3020-d900-8e0371a01938@tarantool.org> Date: Tue, 17 Nov 2020 18:17:58 +0300 MIME-Version: 1.0 In-Reply-To: <85c52e01-c876-8d8e-1543-bf272efb1d79@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 15:42, Serge Petrenko пишет: > > 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 I've addressed the issue in a patch based on your branch, "[PATCH] raft: execute triggers exactly on state change" Take a look, please. > >> 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