Re: [Tarantool-patches] [PATCH 1/3] replication: print number of txs in limbo before its clear

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

On 09.07.2020 19:16, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> ---
>  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[] = {
>