Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Petrenko <sergepetrenko@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: Tue, 27 Jul 2021 01:50:18 +0200
Message-ID: <cce00067-f43b-0382-7470-62658da77b11@tarantool.org> (raw)
In-Reply-To: <036d7d32-54bc-3239-9291-d713b062f324@tarantool.org>

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.

>          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.

> 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.

====================
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();

  reply	other threads:[~2021-07-26 23:50 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 [this message]
2021-07-29 20:56             ` Sergey Petrenko via Tarantool-patches
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=cce00067-f43b-0382-7470-62658da77b11@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=sergepetrenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git