From: Sergey Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v3 07/12] box: introduce `box.ctl.demote` Date: Thu, 29 Jul 2021 23:56:45 +0300 [thread overview] Message-ID: <b3a7cfcb-1a61-8657-7e36-56d5e1a49458@tarantool.org> (raw) In-Reply-To: <cce00067-f43b-0382-7470-62658da77b11@tarantool.org> 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();
next prev parent reply other threads:[~2021-07-29 20:56 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-28 22:12 [Tarantool-patches] [PATCH v3 00/12] forbid implicit limbo ownership transition Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 01/12] replication: always send raft state to subscribers Serge Petrenko via Tarantool-patches 2021-07-04 12:12 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-09 9:43 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 02/12] txn_limbo: fix promote term filtering Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 03/12] raft: refactor raft_new_term() Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 04/12] box: make promote always bump the term Serge Petrenko via Tarantool-patches 2021-07-04 12:14 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:26 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 05/12] replication: forbid implicit limbo owner transition Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 06/12] box: allow calling promote on a candidate Serge Petrenko via Tarantool-patches 2021-07-04 12:14 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:26 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 07/12] box: introduce `box.ctl.demote` Serge Petrenko via Tarantool-patches 2021-07-04 12:27 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:28 ` Serge Petrenko via Tarantool-patches 2021-07-21 23:28 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-23 7:44 ` Sergey Petrenko via Tarantool-patches 2021-07-26 23:50 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-29 20:56 ` Sergey Petrenko via Tarantool-patches [this message] 2021-08-01 16:19 ` Vladislav Shpilevoy via Tarantool-patches 2021-08-03 7:56 ` Serge Petrenko via Tarantool-patches 2021-08-03 23:25 ` Vladislav Shpilevoy via Tarantool-patches 2021-08-04 13:08 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 08/12] txn_limbo: persist the latest effective promote in snapshot Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 09/12] replication: encode version in JOIN request Serge Petrenko via Tarantool-patches 2021-07-04 12:27 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:28 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 10/12] replication: add META stage to JOIN Serge Petrenko via Tarantool-patches 2021-07-04 12:28 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:28 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 11/12] replication: send latest effective promote in initial join Serge Petrenko via Tarantool-patches 2021-07-04 12:28 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:28 ` Serge Petrenko via Tarantool-patches 2021-06-28 22:12 ` [Tarantool-patches] [PATCH v3 12/12] replication: send current Raft term in join response Serge Petrenko via Tarantool-patches 2021-07-04 12:29 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-14 18:28 ` Serge Petrenko via Tarantool-patches 2021-08-04 22:41 ` [Tarantool-patches] [PATCH v3 00/12] forbid implicit limbo ownership transition Vladislav Shpilevoy via Tarantool-patches 2021-08-06 7:54 ` Vitaliia Ioffe via Tarantool-patches 2021-08-06 8:31 ` Kirill Yukhin via Tarantool-patches 2021-08-08 10:46 ` Vladislav Shpilevoy via Tarantool-patches 2021-08-09 7:14 ` Kirill Yukhin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b3a7cfcb-1a61-8657-7e36-56d5e1a49458@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v3 07/12] box: introduce `box.ctl.demote`' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox