From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 F3225445320 for ; Tue, 21 Jul 2020 00:59:17 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Mon, 20 Jul 2020 23:59:14 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/3] replication: print number of txs in limbo before its clear List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org, sergepetrenko@tarantool.org, gorcunov@gmail.com, lvasiliev@tarantool.org Hi! Thanks for the patch! On 09.07.2020 19:16, sergeyb@tarantool.org wrote: > From: Sergey Bronnikov > > --- > src/box/box.cc | 10 ++++++---- > src/box/box.h | 2 +- > src/box/lua/ctl.c | 5 +++-- > 3 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 749c96ca1..a400ed551 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -946,15 +946,15 @@ box_set_replication_anon(void) > > } > > -void > +int > box_clear_synchro_queue(void) There are 3 reasons why it won't/doesn't work: - Here you count not the transactions, but the replica count. 'len' is not queue size in this function. - Functions should set an error in the diagnostics area. Here you silently return -1, -2. Moreover, in Lua we never use 0/-1 notation. - I don't think I want this exposed to users. And there is no way to change this function without affecting the users. Also there are ways how to check that the queue was actually cleared (or not), depending on what exactly you want to test. See my comments in the other commits. Talking of other purposes, such as statistics - yes, we can try to think of something. Queue size would be a useful stat, to see if it grows too fast, or whatsoever. But it should not be available only at the queue clean. It should be constantly available somewhere in box.info. You can provide your ideas here: https://github.com/tarantool/tarantool/issues/5191 > { > if (!is_box_configured || txn_limbo_is_empty(&txn_limbo)) > - return; > + return -1; > uint32_t former_leader_id = txn_limbo.instance_id; > assert(former_leader_id != REPLICA_ID_NIL); > if (former_leader_id == instance_id) > - return; > + return -2; > > /* Wait until pending confirmations/rollbacks reach us. */ > double timeout = 2 * txn_limbo_confirm_timeout(&txn_limbo); > @@ -965,9 +965,9 @@ box_clear_synchro_queue(void) > fiber_sleep(0.001); > } > > + int len = 0; > if (!txn_limbo_is_empty(&txn_limbo)) { > int64_t lsns[VCLOCK_MAX]; > - int len = 0; > const struct vclock *vclock; > replicaset_foreach(replica) { > if (replica->relay != NULL && > @@ -993,6 +993,8 @@ box_clear_synchro_queue(void) > txn_limbo_force_empty(&txn_limbo, confirm_lsn); > assert(txn_limbo_is_empty(&txn_limbo)); > } > + > + return len; > } > > void > diff --git a/src/box/box.h b/src/box/box.h > index 5c4a5ed78..608e26b83 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -258,7 +258,7 @@ extern "C" { > > typedef struct tuple box_tuple_t; > > -void box_clear_synchro_queue(void); > +int box_clear_synchro_queue(void); > > /* box_select is private and used only by FFI */ > API_EXPORT int > diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c > index 2017ddc18..c3cf44f0c 100644 > --- a/src/box/lua/ctl.c > +++ b/src/box/lua/ctl.c > @@ -82,8 +82,9 @@ static int > lbox_ctl_clear_synchro_queue(struct lua_State *L) > { > (void) L; > - box_clear_synchro_queue(); > - return 0; > + int len = box_clear_synchro_queue(); > + lua_pushinteger(L, len); > + return 1; > } > > static const struct luaL_Reg lbox_ctl_lib[] = { >