Re: [Tarantool-patches] [PATCH 08/11] qsync: txn_limbo_wait_confirm -- refactor code a bit

[Serge Petrenko <sergepetrenko@tarantool.org>]

12.11.2020 22:51, Cyrill Gorcunov пишет:
>   - no need for useless comments which describe what
>     we're doing, it is obvious from the code, instead
>     put comment explaining "why" we're doing these
>     things;
>   - use designated assignments for cwp, this is a rule
>     of thumb for C where structure need to be initialized
>     from the scratch;
>   - reorder triggers declaration mess, without empty lines
>     which are ordering context they are simply unreadable;
>   - try to not use goto/jumps when possible (gotos are
>     suitable as a common error entry point but here we
>     can handle everything inside cycle itself).
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Thanks for the patch!

LGTM

>   src/box/txn_limbo.c | 54 ++++++++++++++++++++++-----------------------
>   1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index b58b9647d..cf6122360 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -573,20 +573,25 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
>   	if (txn_limbo_is_empty(limbo))
>   		return 0;
>   
> -	/* initialization of a waitpoint. */
> -	struct confirm_waitpoint cwp;
> -	cwp.caller = fiber();
> -	cwp.is_confirm = false;
> -	cwp.is_rollback = false;
> -
> -	/* Set triggers for the last limbo transaction. */
> -	struct trigger on_complete;
> +	struct confirm_waitpoint cwp = {
> +		.caller = fiber(),
> +		.is_confirm = false,
> +		.is_rollback = false,
> +	};
> +
> +	/*
> +	 * Since we're waiting for all sync transactions to complete,
> +	 * we need the last entry from the limbo.
> +	 */
> +	struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
> +
> +	struct trigger on_complete, on_rollback;
>   	trigger_create(&on_complete, txn_commit_cb, &cwp, NULL);
> -	struct trigger on_rollback;
>   	trigger_create(&on_rollback, txn_rollback_cb, &cwp, NULL);
> -	struct txn_limbo_entry *tle = txn_limbo_last_entry(limbo);
> +
>   	txn_on_commit(tle->txn, &on_complete);
>   	txn_on_rollback(tle->txn, &on_rollback);
> +
>   	double start_time = fiber_clock();
>   	while (true) {
>   		double deadline = start_time + replication_synchro_timeout;
> @@ -594,25 +599,18 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo)
>   		double timeout = deadline - fiber_clock();
>   		int rc = fiber_cond_wait_timeout(&limbo->wait_cond, timeout);
>   		fiber_set_cancellable(cancellable);
> -		if (cwp.is_confirm || cwp.is_rollback)
> -			goto complete;
> -		if (rc != 0)
> -			goto timed_out;
> -	}
> -timed_out:
> -	/* Clear the triggers if the timeout has been reached. */
> -	trigger_clear(&on_complete);
> -	trigger_clear(&on_rollback);
> -	diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
> -	return -1;
> -
> -complete:
> -	if (!cwp.is_confirm) {
> -		/* The transaction has been rolled back. */
> -		diag_set(ClientError, ER_SYNC_ROLLBACK);
> -		return -1;
> +		if (cwp.is_confirm) {
> +			return 0;
> +		} else if (cwp.is_rollback) {
> +			diag_set(ClientError, ER_SYNC_ROLLBACK);
> +			return -1;
> +		} else if (rc != 0) {
> +			trigger_clear(&on_complete);
> +			trigger_clear(&on_rollback);
> +			diag_set(ClientError, ER_SYNC_QUORUM_TIMEOUT);
> +			return -1;
> +		}
>   	}
> -	return 0;
>   }
>   
>   int

-- 
Serge Petrenko