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 7301D6EC55; Thu, 29 Jul 2021 23:56:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7301D6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627592209; bh=nxDlpoLZ9jZ5FvlJTgvC+C6eUCm/Zzg05O4Byd73+74=; 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=FasTkRIfcBRJcT/CZi2zMqv+tuhh+HCV66k0RCGXlLefyNo2p+Ta4pWdIAaQvH8hR R60y2hs9D8eAwbyo7eXrSjKVXNk0zZ/H/EevMCdmi8VBWaEt9kOotQ+yxOwZfqmeQv HLaHQOduMmibjfl1f33OP7i0IJGPTmzhK7dcslQA= Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 791BE6EC55 for ; Thu, 29 Jul 2021 23:56:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 791BE6EC55 Received: by smtp41.i.mail.ru with esmtpa (envelope-from ) id 1m9D5C-0005wZ-GJ; Thu, 29 Jul 2021 23:56:47 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <3b4f7752-f68b-afce-c83a-355bcfbc28cc@tarantool.org> <9ae5765f-ffcb-af66-469b-b0a99d71c331@tarantool.org> <036d7d32-54bc-3239-9291-d713b062f324@tarantool.org> Message-ID: Date: Thu, 29 Jul 2021 23:56:45 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B2770D7874BA03B4AE182A05F53808504076E75D2AED78CDA2E75B61676C6871428B2719E7D5F3ACEABA1FC68D6A8CA706 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79207F2B4714610D0EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D7045943A292EC88638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85BE9FF6653230B880B9C6C70EB742CF3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD186FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0AC5B80A05675ACD1421F57CABF51D54D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3FA486DC37A503D0B03F1AB874ED89028C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637149D0840703ADBE5EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79DB53CE84373687089170F8FB043696003 X-C1DE0DAB: 0D63561A33F958A5BFE28D9D74FE0980524AABA55D179B5F6F17E5C4A3787D96D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7536C62C4FBC402878410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FF70CCB59A6ED8707C0878FE2B21D9D2D8958DD89AFAD0CDE371FF2E6EF294A2DEC0A1D43BE9F3E41D7E09C32AA3244C46DFC412B23C1CF1231C964DF1B897BEB4DF56057A86259FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPp/mPgZxawHNhgeyUjjwJw== X-Mailru-Sender: 9482C2B233321BD27EE33DDA534ACD91D0F639BA0513F86D3C5D78F2EDE4D435570FBBDF5DF207066BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 07/12] box: introduce `box.ctl.demote` 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: Sergey Petrenko via Tarantool-patches Reply-To: Sergey Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 27.07.2021 02:50, Vladislav Shpilevoy пишет: > Thanks for the fixes! > > On 23.07.2021 09:44, Sergey Petrenko wrote: >> 22.07.2021 01:28, Vladislav Shpilevoy пишет: >>> Thanks for the fixes! >>> >>>>> 2. On top of this commit and on top of the branch too I tried to >>>>> promote a candidate and got a strange error in the logs, although >>>>> the promotion was successful: >>>>> >>>>> -- >>>>> -- Instance 1 >>>>> -- >>>>> >>>>> -- Step 1 >>>>> box.cfg{ >>>>>       listen = 3313, >>>>>       election_mode = 'candidate', >>>>>       replication_synchro_quorum = 2, >>>>>       replication = {'localhost:3314', 'localhost:3313'} >>>>> } >>>>> box.schema.user.grant('guest', 'super') >>>>> >>>>> >>>>> -- >>>>> -- Instance 2 >>>>> -- >>>>> >>>>> -- Step 2 >>>>> box.cfg{ >>>>>       listen = 3314, >>>>>       election_mode = 'voter', >>>>>       replication_synchro_quorum = 2, >>>>>       replication = {'localhost:3314', 'localhost:3313'}, >>>>>       read_only = true, >>>>> } >>>>> >>>>> -- Step 3 >>>>> box.cfg{read_only = false, election_mode = 'candidate'} >>>>> >>>>> -- Step 4 >>>>> box.ctl.promote() >>>>> >>>>> main/112/raft_worker box.cc:1538 E> ER_UNSUPPORTED: box.ctl.promote does not support simultaneous invocations >>>>> --- >>>>> ... >>>> That's because once a candidate becomes the leader, it tries to issue `box.ctl.promote()`, and fails, >>>> since we're already in `box.ctl.promote()` call. >>>> I'm not sure how to handle that properly. This doesn't break anything though. >>> Still, the error message looks really not good. There is an option - >>> make box_promote() for candidate node just call raft_promote() and set >>> box_in_promote = false. Then wait for the term outcome. Will it work? >>> You would need to rebase your patchset on master branch then. >> Hmm, I don't like that. This will work, but it will complicate things in >> >> box_promote even more. Now one has to keep in mind that a manual >> >> promote call has 2 phases, one when it is issued manually and the other >> >> when it's called automatically when (and if) the instance becomes the leader. > But is it different in the new version on the branch? You still need to check > if you are "in promote", but instead of it being encapsulated into box_promote() > we now need to check it potentially in all box_promote() usage places. Which is > only 2 now though, and only 1 needs this. But you can see that now you need to > think if you should ignore being already in promote. >> diff --git a/src/box/box.cc b/src/box/box.cc >> index a30e4f78d..5cdca4bd4 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1692,20 +1692,24 @@ box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn) >>  } >> >>  /* A guard to block multiple simultaneous promote()/demote() invocations. */ >> -static bool box_in_promote = false; >> +static bool in_box_promote = false; >> + >> +bool box_in_promote(void) { > Could you please put the return value on a separate line and use 'is' > as a suffix? For instance, `box_is_in_promote()`. >> diff --git a/src/box/box.h b/src/box/box.h >> index aaf20d9dd..344ed90f2 100644 >> --- a/src/box/box.h >> +++ b/src/box/box.h >> @@ -88,11 +88,11 @@ box_raft_update_synchro_queue(struct raft *raft) >>  { >>      assert(raft == box_raft()); >>      /* >> -     * In case these are manual elections, we are already in the middle of a >> -     * `promote` call. No need to call it once again. >> +     * In case the elections were triggered manually, we are already in >> +     * the middle of a `promote` call. No need to call it once again. >>       */ >>      if (raft->state == RAFT_STATE_LEADER && >> -        box_election_mode != ELECTION_MODE_MANUAL) { >> +        !box_in_promote()) { > It just seems to me that it should not be of raft's business if it was > promoted manually or not. Ok, I see your point. I'll apply your diff. >>          int rc = 0; >>          uint32_t errcode = 0; >>          do { >> ===================================== >> >> >>> It might be a little easier to do if you apply the diff below. (Warning: >>> I didn't test it.) The motivation is that one of the main reasons why I >>> wanted box_promote() simplified was because of the strange meaning of some >>> flags. In particular, try_wait flag somewhy triggered elections before the >>> waiting which is super not obvious why. How does 'wait' come to 'elections'? >>> >>> In the diff I tried to remove these flags entirely. And now you have a >>> single place in the code of box_promote(), where ELECTION_MODE_CANDIDATE >>> stuff is handled. Here you could try the proposal I gave above. >> Thanks for the help! Your diff looks good, I've reworked my patches to comply. > Wouldn't it be too hard to merge this diff into the previous > commits? Because it could be done like that from the first > box_promote-refactoring commit it looks to me. No problem. >> diff --git a/src/box/box.cc b/src/box/box.cc >> index af39f66f4..36302310b 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1701,14 +1701,14 @@ box_promote(void) >> if (!is_box_configured) >> return 0; >> if (txn_limbo_replica_term(&txn_limbo, instance_id) == >> - box_raft()->term) >> + raft->term) >> return 0; >> - bool run_elections = false; >> - bool try_wait = false; >> - >> switch (box_election_mode) { >> case ELECTION_MODE_OFF: >> - try_wait = true; >> + if (box_try_wait_confirm(2 * replication_synchro_timeout) != 0) >> + return -1; >> + if (box_trigger_elections() != 0) >> + return -1; >> break; >> case ELECTION_MODE_VOTER: >> assert(raft->state == RAFT_STATE_FOLLOWER); >> @@ -1717,23 +1717,17 @@ box_promote(void) >> return -1; >> case ELECTION_MODE_MANUAL: >> case ELECTION_MODE_CANDIDATE: >> - run_elections = raft->state != RAFT_STATE_LEADER; >> + if (raft->state != RAFT_STATE_LEADER && >> + box_run_elections() != 0) { >> + return -1; >> + } >> break; >> default: >> unreachable(); >> } >> >> - int64_t wait_lsn = -1; >> - >> - if (run_elections && box_run_elections() != 0) >> - return -1; >> - if (try_wait) { >> - if (box_try_wait_confirm(2 * replication_synchro_timeout) != 0) >> - return -1; >> - if (box_trigger_elections() != 0) >> - return -1; >> - } >> - if ((wait_lsn = box_wait_limbo_acked()) < 0) >> + int64_t wait_lsn = box_wait_limbo_acked(); >> + if (wait_lsn < 0) >> return -1; >> >> box_issue_promote(txn_limbo.owner_id, wait_lsn); > I thought more about how the manual promotion conflicts with the > automatic one and I tried to separate them. See my diff below, it > is done on top of this commit, but I didn't push it, because I > didn't run the tests except the one which used to log an error > in the first version of the patchset. > > Thanks to the new small helper functions, I implemented a special > function for automatic promote which does not conflict with > the manual one. It you will apply it though, you might want to > do it earlier that this commit. Thanks for the help! I've applied your diff with minor changes to patches box: split manual and automatic promotion box: allow calling promote on a candidate > > ==================== > diff --git a/src/box/box.cc b/src/box/box.cc > index 36302310b..65d064615 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1599,7 +1599,7 @@ box_try_wait_confirm(double timeout) > * Return lsn of the last limbo entry on success, -1 on error. > */ > static int64_t > -box_wait_limbo_acked(void) > +box_wait_limbo_acked(double timeout) > { > if (txn_limbo_is_empty(&txn_limbo)) > return txn_limbo.confirmed_lsn; > @@ -1629,7 +1629,7 @@ box_wait_limbo_acked(void) > int64_t wait_lsn = last_entry->lsn; > > if (box_wait_quorum(txn_limbo.owner_id, wait_lsn, quorum, > - replication_synchro_timeout) != 0) > + timeout) != 0) > return -1; > > if (box_check_promote_term_intact(promote_term) != 0) > @@ -1675,9 +1675,23 @@ box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn) > /* A guard to block multiple simultaneous box_promote() invocations. */ > static bool in_box_promote = false; > > -bool box_in_promote(void) > +int > +box_promote_qsync(void) > { > - return in_box_promote; > + assert(!in_box_promote); > + assert(is_box_configured); > + struct raft *raft = box_raft(); > + if (raft->state != RAFT_STATE_LEADER) > + return 0; > + assert(box_election_mode == ELECTION_MODE_MANUAL || > + box_election_mode == ELECTION_MODE_CANDIDATE); > + if (txn_limbo_replica_term(&txn_limbo, instance_id) == raft->term) > + return 0; > + int64_t wait_lsn = box_wait_limbo_acked(TIMEOUT_INFINITY); > + if (wait_lsn < 0) > + return -1; > + box_issue_promote(txn_limbo.owner_id, wait_lsn); > + return 0; > } > > int > @@ -1700,8 +1714,8 @@ box_promote(void) > */ > if (!is_box_configured) > return 0; > - if (txn_limbo_replica_term(&txn_limbo, instance_id) == > - raft->term) > + if (txn_limbo_replica_term(&txn_limbo, instance_id) == raft->term || > + raft->state == RAFT_STATE_LEADER) > return 0; > switch (box_election_mode) { > case ELECTION_MODE_OFF: > @@ -1715,18 +1729,16 @@ box_promote(void) > diag_set(ClientError, ER_UNSUPPORTED, "election_mode='voter'", > "manual elections"); > return -1; > - case ELECTION_MODE_MANUAL: > case ELECTION_MODE_CANDIDATE: > - if (raft->state != RAFT_STATE_LEADER && > - box_run_elections() != 0) { > - return -1; > - } > - break; > + case ELECTION_MODE_MANUAL: > + /* Let the automatic elections finish the promotion properly. */ > + in_box_promote = false; > + return box_run_elections(); > default: > unreachable(); > } > > - int64_t wait_lsn = box_wait_limbo_acked(); > + int64_t wait_lsn = box_wait_limbo_acked(replication_synchro_timeout); > if (wait_lsn < 0) > return -1; > > diff --git a/src/box/box.h b/src/box/box.h > index db636b058..5e5d0bf6d 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -273,12 +273,12 @@ extern "C" { > > typedef struct tuple box_tuple_t; > > -bool > -box_in_promote(void); > - > int > box_promote(void); > > +int > +box_promote_qsync(void); > + > /* box_select is private and used only by FFI */ > API_EXPORT int > box_select(uint32_t space_id, uint32_t index_id, > diff --git a/src/box/raft.c b/src/box/raft.c > index 5e496c2e4..07501157f 100644 > --- a/src/box/raft.c > +++ b/src/box/raft.c > @@ -83,30 +83,6 @@ box_raft_request_to_msg(const struct raft_request *req, struct raft_msg *msg) > }; > } > > -static void > -box_raft_update_synchro_queue(struct raft *raft) > -{ > - assert(raft == box_raft()); > - /* > - * In case the elections were triggered manually, we are already in > - * the middle of a `promote` call. No need to call it once again. > - */ > - if (raft->state == RAFT_STATE_LEADER && > - !box_in_promote()) { > - int rc = 0; > - uint32_t errcode = 0; > - do { > - rc = box_promote(); > - if (rc != 0) { > - struct error *err = diag_last_error(diag_get()); > - errcode = box_error_code(err); > - diag_log(); > - } > - } while (rc != 0 && errcode == ER_QUORUM_WAIT && > - !fiber_is_cancelled()); > - } > -} > - > static int > box_raft_worker_f(va_list args) > { > @@ -117,7 +93,12 @@ box_raft_worker_f(va_list args) > box_raft_has_work = false; > > raft_process_async(raft); > - box_raft_update_synchro_queue(raft); > + /* > + * XXX: perhaps it should not ever fail. Or at least need a > + * proper support for failures instead of the ignorance. > + */ > + if (box_promote_qsync() != 0) > + diag_log(); > > if (!box_raft_has_work) > fiber_yield();