Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>,
	tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v9 3/5] limbo: order access to the limbo terms
Date: Tue, 3 Aug 2021 01:48:18 +0200	[thread overview]
Message-ID: <4afbed2a-36cd-9156-ccb6-d218a0db395a@tarantool.org> (raw)
In-Reply-To: <20210730113539.563318-4-gorcunov@gmail.com>

Hi! Thanks for the patch!

See 6 comments below.

> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index f621fa657..a7f472714 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -909,12 +910,15 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
>  	 * transactions side, including the async ones.
>  	 */
>  	if (journal_write(&entry.base) != 0)
> -		goto err;
> +		goto err_unlock;
>  	if (entry.base.res < 0) {
>  		diag_set_journal_res(entry.base.res);
> -		goto err;
> +		goto err_unlock;
>  	}
> +	txn_limbo_term_unlock(&txn_limbo);
>  	return 0;
> +err_unlock:
> +	txn_limbo_term_unlock(&txn_limbo);

1. Could be done simpler:

====================
@@ -908,7 +909,9 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
 	 * before trying to commit. But that requires extra steps from the
 	 * transactions side, including the async ones.
 	 */
-	if (journal_write(&entry.base) != 0)
+	int rc = journal_write(&entry.base);
+	txn_limbo_term_unlock(&txn_limbo);
+	if (rc != 0)
 		goto err;
 	if (entry.base.res < 0) {
 		diag_set_journal_res(entry.base.res);
====================

> diff --git a/src/box/box.cc b/src/box/box.cc
> index 535f30292..5ca617e32 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1573,7 +1573,7 @@ box_run_elections(void)
>  static int
>  box_check_promote_term_intact(uint64_t promote_term)
>  {
> -	if (txn_limbo.promote_greatest_term != promote_term) {
> +	if (txn_limbo_term_max_raw(&txn_limbo) != promote_term) {

2. In raft terminology we call such data 'volatile' instead
of 'raw'.

>  		diag_set(ClientError, ER_INTERFERING_PROMOTE,
>  			 txn_limbo.owner_id);
>  		return -1;
> @@ -1728,7 +1728,7 @@ box_promote(void)
>  	 * Currently active leader (the instance that is seen as leader by both
>  	 * raft and txn_limbo) can't issue another PROMOTE.
>  	 */
> -	bool is_leader = txn_limbo_replica_term(&txn_limbo, instance_id) ==
> +	bool is_leader = txn_limbo_term(&txn_limbo, instance_id) ==

3. Why did you change the name? The old one was closer to reality. When
you say 'limbo term', it is not clear whether you mean the max term or
term of one of the nodes.

Please, extract all such renames into a separate commit. Otherwise
it is harder to find the functional changes in this one.

> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 570f77c46..be5e0adf5 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -724,22 +725,23 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double timeout)
>  }
>  
>  void
> -txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
> +txn_limbo_process_locked(struct txn_limbo *limbo,
> +			 const struct synchro_request *req)
>  {

4. Please, add an assertion that the latch is occupied. The same for the
other _locked functions.

>  	uint64_t term = req->term;
>  	uint32_t origin = req->origin_id;
> -	if (txn_limbo_replica_term(limbo, origin) < term) {
> +	if (txn_limbo_term_locked(limbo, origin) < term) {
>  		vclock_follow(&limbo->promote_term_map, origin, term);
> -		if (term > limbo->promote_greatest_term)
> -			limbo->promote_greatest_term = term;
> +		if (term > limbo->promote_term_max)
> +			limbo->promote_term_max = term;
>  	} else if (iproto_type_is_promote_request(req->type) &&
> -		   limbo->promote_greatest_term > 1) {
> +		   limbo->promote_term_max > 1) {
>  		/* PROMOTE for outdated term. Ignore. */
>  		say_info("RAFT: ignoring %s request from instance "
>  			 "id %u for term %llu. Greatest term seen "
>  			 "before (%llu) is bigger.",
>  			 iproto_type_name(req->type), origin, (long long)term,
> -			 (long long)limbo->promote_greatest_term);
> +			 (long long)limbo->promote_term_max);
>  		return;
>  	}
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index 53e52f676..25faffd2b 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -211,14 +216,61 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
>  				in_queue);
>  }
>  
> +/** Lock promote data. */
> +static inline void
> +txn_limbo_term_lock(struct txn_limbo *limbo)
> +{
> +	latch_lock(&limbo->promote_latch);
> +}
> +
> +/** Unlock promote data. */
> +static void

5. Why isn't it inline while the others are?

> +txn_limbo_term_unlock(struct txn_limbo *limbo)
> +{
> +	latch_unlock(&limbo->promote_latch);
> +}
> +
> +/** Test if promote data is locked. */
> +static inline bool
> +txn_limbo_term_is_locked(const struct txn_limbo *limbo)
> +{
> +	return latch_is_locked(&limbo->promote_latch);
> +}
> +
> +/** Fetch replica's term with lock taken. */
> +static inline uint64_t
> +txn_limbo_term_locked(struct txn_limbo *limbo, uint32_t replica_id)
> +{
> +	panic_on(!txn_limbo_term_is_locked(limbo),
> +		 "limbo: unlocked term read for replica_id %u",
> +		 replica_id);

6. This new macro seems counter-intuitive because works vice versa
compared to assert(). Can you try to rename it somehow and make it
accept a condition which must be true instead of false?

> +	return vclock_get(&limbo->promote_term_map, replica_id);
> +}

  reply	other threads:[~2021-08-02 23:48 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 11:35 [Tarantool-patches] [PATCH v9 0/5] limbo: implement packets filtering Cyrill Gorcunov via Tarantool-patches
2021-07-30 11:35 ` [Tarantool-patches] [PATCH v9 1/5] latch: add latch_is_locked helper Cyrill Gorcunov via Tarantool-patches
2021-07-30 11:35 ` [Tarantool-patches] [PATCH v9 2/5] say: introduce panic_on helper Cyrill Gorcunov via Tarantool-patches
2021-07-30 11:35 ` [Tarantool-patches] [PATCH v9 3/5] limbo: order access to the limbo terms Cyrill Gorcunov via Tarantool-patches
2021-08-02 23:48   ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-08-03 11:23     ` Cyrill Gorcunov via Tarantool-patches
2021-07-30 11:35 ` [Tarantool-patches] [PATCH v9 4/5] limbo: filter incoming synchro requests Cyrill Gorcunov via Tarantool-patches
2021-08-02 23:50   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-03 13:25     ` Cyrill Gorcunov via Tarantool-patches
2021-08-03 10:51   ` Serge Petrenko via Tarantool-patches
2021-08-03 13:49     ` Cyrill Gorcunov via Tarantool-patches
2021-07-30 11:35 ` [Tarantool-patches] [PATCH v9 5/5] test: add replication/gh-6036-rollback-confirm Cyrill Gorcunov 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=4afbed2a-36cd-9156-ccb6-d218a0db395a@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v9 3/5] limbo: order access to the limbo terms' \
    /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