Tarantool development patches archive
 help / color / mirror / Atom feed
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
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();

  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 \
    /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