From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 1D69F6FC8F; Fri, 16 Apr 2021 02:30:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1D69F6FC8F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618529459; bh=USRadQ6liY006Mdc8rUIe0Lkvf7U5zHzfYNf1PBA7so=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=hueo4uIG4k4X1uDFg05Y+HpeW+rBd53pS7wHzA3xwgSmkCB5/dFFh1uZWIm0vxvZf RIrdwn7jc/AAf2Bhmam+sZTzw++QeD65HeBdrG/rO1B2q48sB+DooSCEiFFoNPinfO h3OQC2oJHiWZAEH7pAomd6oYE3iUNPox0lYfpBuA= Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 488006FC8F for ; Fri, 16 Apr 2021 02:30:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 488006FC8F Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1lXBRn-0002hI-FI; Fri, 16 Apr 2021 02:30:56 +0300 To: Serge Petrenko , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: Message-ID: Date: Fri, 16 Apr 2021 01:30:54 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D5E28B957962BB550182A05F5380850404B0EC436D30F0F0DA6C58FB226E63ECE87CFF4ACB862FDEEFFA7FB57DA64A786 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76C33606E6E3377E0EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379F6495389D012EA98638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B25DA58EEFD608DCE55C8F01DCC4C80C4DE8578C74B6F99271D2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7E4DF6D1C10F22F599FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE760B0E0D110DE89CDD32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C066BDD992D71BA80ACD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C34B08FA16E56A400835872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824A2A04A2ABAA09D25379311020FFC8D4AD6C30AE2B5E62355BB6E268B2E08ECDEE X-C1DE0DAB: 0D63561A33F958A598DE1E90AC30BEA9CA65FFD685418E9547C350C5D6072177D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AF12ADB97C97CD89CF504C9722B09EDDA9D2E887DDB820E67D94C34947C6634D8F7F4BA4038BC9C81D7E09C32AA3244C7A84EEAEA5679DFBDFDC2ADA9824824881560E2432555DBBFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3S6P1v0GIqQCTZFQNZ0mZw== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA311032A1953DF44778D125FAF8868A1262B1CE636541CB04341207784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 08/10] Support manual elections in `box.ctl.clear_synchro_queue()` X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for working on this! See 5 comments below. > diff --git a/src/box/box.cc b/src/box/box.cc > index 3729ed997..6c7c8968a 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1525,12 +1526,74 @@ box_clear_synchro_queue(bool try_wait) > if (!is_box_configured || > raft_source_term(box_raft(), instance_id) == box_raft()->term) > return 0; > + > + bool run_elections = false; > + > + switch (box_election_mode) { > + case ELECTION_MODE_OFF: > + break; > + case ELECTION_MODE_VOTER: > + assert(box_raft()->state == RAFT_STATE_FOLLOWER); > + diag_set(ClientError, ER_UNSUPPORTED, "election_mode='voter'", > + "manual elections"); > + return -1; > + case ELECTION_MODE_MANUAL: > + assert(box_raft()->state != RAFT_STATE_CANDIDATE); > + if (box_raft()->state == RAFT_STATE_LEADER) { > + diag_set(ClientError, ER_ALREADY_LEADER); > + return -1; > + } > + run_elections = true; > + try_wait = false; > + break; > + case ELECTION_MODE_CANDIDATE: > + /* > + * Leader elections are enabled, and this instance is allowed to > + * promote only if it's already an elected leader. No manual > + * elections. > + */ > + if (box_raft()->state != RAFT_STATE_LEADER) { 1. That is strange. Why do you allow to promote the node if it is already the leader when mode is candidate, but do not allow the same when the mode is manual? Shouldn't we throw an error when the mode is candidate regardless of the node role? > + diag_set(ClientError, ER_UNSUPPORTED, "election_mode=" > + "'candidate'", "manual elections"); > + return -1; > + } > + break; > + default: > + unreachable(); > + } > + > uint32_t former_leader_id = txn_limbo.owner_id; > int64_t wait_lsn = txn_limbo.confirmed_lsn; > int rc = 0; > int quorum = replication_synchro_quorum; > in_clear_synchro_queue = true; > > + if (run_elections) { > + /* > + * Make this instance a candidate and run until some leader, not > + * necessarily this instance, emerges. > + */ > + raft_cfg_is_candidate(box_raft(), true, false); > + /* > + * Trigger new elections without waiting for an old leader to > + * disappear. > + */ > + raft_new_term(box_raft()); > + box_raft_wait_leader_found(); > + raft_cfg_is_candidate(box_raft(), false, false); 2. What if during box_raft_wait_leader_found() I made the node candidate via box.cfg? Won't you then reset it back to non-candidate here? It probably should reset the current box.cfg mode back. Not just remove the candidate flag. > + if (!box_raft()->is_enabled) { > + diag_set(ClientError, ER_RAFT_DISABLED); > + in_clear_synchro_queue = false; > + return -1; > + } > + if (box_raft()->state != RAFT_STATE_LEADER) { > + diag_set(ClientError, ER_INTERFERING_PROMOTE, > + box_raft()->leader); > + in_clear_synchro_queue = false; > + return -1; > + } > + } > + > if (txn_limbo_is_empty(&txn_limbo)) > goto promote; > > diff --git a/src/box/raft.c b/src/box/raft.c > index 285dbe4fd..c7dc79f9b 100644 > --- a/src/box/raft.c > +++ b/src/box/raft.c > @@ -336,6 +336,28 @@ fail: > panic("Could not write a raft request to WAL\n"); > } > > +static int > +box_raft_wait_leader_found_trig(struct trigger *trig, void *event) 3. I thought we usually call triggers with _f suffix, not _trig. > +{ > + struct raft *raft = (struct raft *)event; > + assert(raft == box_raft()); > + struct fiber *waiter = (struct fiber *)trig->data; 4. No need to cast this and event - void * cast works naturally in C. > + if (raft->leader != REPLICA_ID_NIL || !raft->is_enabled) > + fiber_wakeup(waiter); > + return 0; > +} > diff --git a/src/box/raft.h b/src/box/raft.h > index 15f4e80d9..8fce423e1 100644 > --- a/src/box/raft.h > +++ b/src/box/raft.h > @@ -97,6 +97,9 @@ box_raft_checkpoint_remote(struct raft_request *req); > int > box_raft_process(struct raft_request *req, uint32_t source); > > +void > +box_raft_wait_leader_found(); > + > void > box_raft_init(void); > > diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c > index e9ce8cade..7b77e05ea 100644 > --- a/src/lib/raft/raft.c > +++ b/src/lib/raft/raft.c > @@ -846,7 +846,7 @@ raft_cfg_is_enabled(struct raft *raft, bool is_enabled) > } > > void > -raft_cfg_is_candidate(struct raft *raft, bool is_candidate) > +raft_cfg_is_candidate(struct raft *raft, bool is_candidate, bool demote) 5. I know it might lead to some code duplication, but probably better move that to other functions. For example, raft_cfg_is_temporary_candidate() or something like that. Otherwise it appears surprisingly hard to follow these 2 flags together. Although I might be wrong and it would look worse. Did you try? Or another option: raft_cfg_is_candidate(box_raft(), true, false); raft_cfg_is_candidate(box_raft(), false, false); turns into raft_start_candidate(box_raft()) raft_stop_candidate(box_raft()) Also it would be good to have unit tests for the changes in raft.h and raft.c.