From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 42662469719 for ; Fri, 13 Nov 2020 12:56:38 +0300 (MSK) References: <20201112195121.191366-1-gorcunov@gmail.com> <20201112195121.191366-9-gorcunov@gmail.com> From: Serge Petrenko Message-ID: <06a260e4-8ed3-b803-4142-3d7261700c9a@tarantool.org> Date: Fri, 13 Nov 2020 12:56:37 +0300 MIME-Version: 1.0 In-Reply-To: <20201112195121.191366-9-gorcunov@gmail.com> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 08/11] qsync: txn_limbo_wait_confirm -- refactor code a bit List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Cc: Vladislav Shpilevoy 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 > --- 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