Re: [Tarantool-patches] [RFC v5 3/5] limbo: gather promote tracking into a separate structure

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

15.07.2021 00:23, Cyrill Gorcunov пишет:
> It is needed to introduce ordered promote related data
> modifications in next patch.
>
> Part-of #6036
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Thanks for the patch!
Generally looks good with a couple of comments.

>   src/box/txn_limbo.c | 24 ++++++++++++++--------
>   src/box/txn_limbo.h | 49 ++++++++++++++++++++++++++++-----------------
>   2 files changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 570f77c46..957fe0d1e 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -37,6 +37,13 @@
>   
>   struct txn_limbo txn_limbo;
>   
> +static void
> +txn_limbo_promote_create(struct txn_limbo_promote *pmt)
> +{
> +	vclock_create(&pmt->terms_map);
> +	pmt->terms_max = 0;
> +}
> +

I don't like the name (limbo->promote, struct txn_limbo_promote),
but can't come up with a better one.

The structure doesn't hold a specific promote. It's more like "promote 
history",
or "terms seen".

Maybe something like "term history" ? "term tracker" ?
"remote term set", "term set" ?

Just a suggestion, my names are not too good.

>   static inline void
>   txn_limbo_create(struct txn_limbo *limbo)
>   {
> @@ -45,8 +52,7 @@ txn_limbo_create(struct txn_limbo *limbo)
>   	limbo->owner_id = REPLICA_ID_NIL;
>   	fiber_cond_create(&limbo->wait_cond);
>   	vclock_create(&limbo->vclock);
> -	vclock_create(&limbo->promote_term_map);
> -	limbo->promote_greatest_term = 0;
> +	txn_limbo_promote_create(&limbo->promote);
>   	limbo->confirmed_lsn = 0;
>   	limbo->rollback_count = 0;
>   	limbo->is_in_rollback = false;
> @@ -305,10 +311,11 @@ void
>   txn_limbo_checkpoint(const struct txn_limbo *limbo,
>   		     struct synchro_request *req)
>   {
> +	const struct txn_limbo_promote *pmt = &limbo->promote;
>   	req->type = IPROTO_PROMOTE;
>   	req->replica_id = limbo->owner_id;
>   	req->lsn = limbo->confirmed_lsn;
> -	req->term = limbo->promote_greatest_term;
> +	req->term = pmt->terms_max;
>   }
>   
>   static void
> @@ -726,20 +733,21 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double timeout)
>   void
>   txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
>   {
> +	struct txn_limbo_promote *pmt = &limbo->promote;
>   	uint64_t term = req->term;
>   	uint32_t origin = req->origin_id;
>   	if (txn_limbo_replica_term(limbo, origin) < term) {
> -		vclock_follow(&limbo->promote_term_map, origin, term);
> -		if (term > limbo->promote_greatest_term)
> -			limbo->promote_greatest_term = term;
> +		vclock_follow(&pmt->terms_map, origin, term);
> +		if (term > pmt->terms_max)
> +			pmt->terms_max = term;
>   	} else if (iproto_type_is_promote_request(req->type) &&
> -		   limbo->promote_greatest_term > 1) {
> +		   pmt->terms_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)pmt->terms_max);
>   		return;
>   	}
>   
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index 53e52f676..70a5fbfd5 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -75,6 +75,31 @@ txn_limbo_entry_is_complete(const struct txn_limbo_entry *e)
>   	return e->is_commit || e->is_rollback;
>   }
>   
> +/**
> + * Keep state of promote requests to handle split-brain
> + * situation and other errors.
> + */
> +struct txn_limbo_promote {
> +	/**
> +	 * Latest terms received with PROMOTE entries from remote instances.
> +	 * Limbo uses them to filter out the transactions coming not from the
> +	 * limbo owner, but so outdated that they are rolled back everywhere
> +	 * except outdated nodes.
> +	 */
> +	struct vclock terms_map;
> +	/**
> +	 * The biggest PROMOTE term seen by the instance and persisted in WAL.
> +	 * It is related to raft term, but not the same. Synchronous replication
> +	 * represented by the limbo is interested only in the won elections
> +	 * ended with PROMOTE request.
> +	 * It means the limbo's term might be smaller than the raft term, while
> +	 * there are ongoing elections, or the leader is already known and this
> +	 * instance hasn't read its PROMOTE request yet. During other times the
> +	 * limbo and raft are in sync and the terms are the same.
> +	 */
> +	uint64_t terms_max;
> +};
> +
>   /**
>    * Limbo is a place where transactions are stored, which are
>    * finished, but not committed nor rolled back. These are
> @@ -130,23 +155,9 @@ struct txn_limbo {
>   	 */
>   	struct vclock vclock;
>   	/**
> -	 * Latest terms received with PROMOTE entries from remote instances.
> -	 * Limbo uses them to filter out the transactions coming not from the
> -	 * limbo owner, but so outdated that they are rolled back everywhere
> -	 * except outdated nodes.
> -	 */
> -	struct vclock promote_term_map;
> -	/**
> -	 * The biggest PROMOTE term seen by the instance and persisted in WAL.
> -	 * It is related to raft term, but not the same. Synchronous replication
> -	 * represented by the limbo is interested only in the won elections
> -	 * ended with PROMOTE request.
> -	 * It means the limbo's term might be smaller than the raft term, while
> -	 * there are ongoing elections, or the leader is already known and this
> -	 * instance hasn't read its PROMOTE request yet. During other times the
> -	 * limbo and raft are in sync and the terms are the same.
> +	 * Track promote requests.
>   	 */
> -	uint64_t promote_greatest_term;
> +	struct txn_limbo_promote promote;
>   	/**
>   	 * Maximal LSN gathered quorum and either already confirmed in WAL, or
>   	 * whose confirmation is in progress right now. Any attempt to confirm
> @@ -218,7 +229,8 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
>   static inline uint64_t
>   txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
>   {
> -	return vclock_get(&limbo->promote_term_map, replica_id);
> +	const struct txn_limbo_promote *pmt = &limbo->promote;
> +	return vclock_get(&pmt->terms_map, replica_id);
>   }
>   
>   /**
> @@ -229,8 +241,9 @@ static inline bool
>   txn_limbo_is_replica_outdated(const struct txn_limbo *limbo,
>   			      uint32_t replica_id)
>   {
> +	const struct txn_limbo_promote *pmt = &limbo->promote;
>   	return txn_limbo_replica_term(limbo, replica_id) <
> -	       limbo->promote_greatest_term;
> +	       pmt->terms_max;
>   }
>   
>   /**

-- 
Serge Petrenko